[U-Boot] [PATCH v4 1/2] splash: sort include files
Sort include files in accordance to u-boot coding style.
Signed-off-by: Tomas Melin tomas.melin@vaisala.com ---
Changes in v4: - Added missing changelog
Changes in v3: - Change patch order so that include sort patch comes prior to adding new include
Changes in v2: - Add separate patch for sorting include files
common/splash_source.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/common/splash_source.c b/common/splash_source.c index d300e46..70d724f 100644 --- a/common/splash_source.c +++ b/common/splash_source.c @@ -7,15 +7,16 @@ */
#include <common.h> -#include <nand.h> +#include <bmp_layout.h> #include <errno.h> -#include <splash.h> -#include <spi_flash.h> +#include <fs.h> +#include <image.h> +#include <nand.h> +#include <sata.h> #include <spi.h> +#include <spi_flash.h> +#include <splash.h> #include <usb.h> -#include <sata.h> -#include <bmp_layout.h> -#include <fs.h>
DECLARE_GLOBAL_DATA_PTR;
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 ---
Changes in v4: - Added missing changelog
Changes in v3: - Add documentation to README.splashprepare - Change printf() to debug() - Coding style fixes
Changes in v2: - Add helper functions to image-fit.c for getting required FIT data fields - Add comments
common/image-fit.c | 48 ++++++++++++++++++++++++++++++++ common/splash_source.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++ doc/README.splashprepare | 15 ++++++---- include/image.h | 4 +++ include/splash.h | 5 ++-- 5 files changed, 136 insertions(+), 7 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c index 25f8a11..8d9d050 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -775,6 +775,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 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 @@ -10,6 +10,7 @@ #include <bmp_layout.h> #include <errno.h> #include <fs.h> +#include <fdt_support.h> #include <image.h> #include <nand.h> #include <sata.h> @@ -241,6 +242,72 @@ 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) { + 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; + } + + 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; + } + + /* 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 {
On 5 December 2016 at 02:36, 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
Changes in v4:
- Added missing changelog
Changes in v3:
- Add documentation to README.splashprepare
- Change printf() to debug()
- Coding style fixes
Changes in v2:
- Add helper functions to image-fit.c for getting required FIT data fields
- Add comments
common/image-fit.c | 48 ++++++++++++++++++++++++++++++++ common/splash_source.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++ doc/README.splashprepare | 15 ++++++---- include/image.h | 4 +++ include/splash.h | 5 ++-- 5 files changed, 136 insertions(+), 7 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
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@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 {
Hi Igor,
On 11 December 2016 at 10:37, Igor Grinberg grinberg@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@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
On 12/11/16 22:27, Simon Glass wrote:
Hi Igor,
On 11 December 2016 at 10:37, Igor Grinberg grinberg@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@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).
Unless, there a kind of corruption or a user mistake and then that same user can't even understand what happened because we do not help him with an error message. You cannot know that these error messages will never happen... This is a generic code which can be used by a wide variety of platforms - I don't think you can foresee all the possible use cases.
We are talking about several tens of bytes vs. usability. If there is an error, it should be stated as such. It should not just exit silently...
I think that debug() is a good thing to use, but we shouldn't be obsessed with it.
So long as the error is reported (even if it is not a very specific error), people can add DEBUG and track it down.
That depends who 'people' are? Devs? Users?
Hi Igor,
On 12 December 2016 at 01:37, Igor Grinberg grinberg@compulab.co.il wrote:
On 12/11/16 22:27, Simon Glass wrote:
Hi Igor,
On 11 December 2016 at 10:37, Igor Grinberg grinberg@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@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).
Unless, there a kind of corruption or a user mistake and then that same user can't even understand what happened because we do not help him with an error message. You cannot know that these error messages will never happen... This is a generic code which can be used by a wide variety of platforms - I don't think you can foresee all the possible use cases.
We are talking about several tens of bytes vs. usability. If there is an error, it should be stated as such. It should not just exit silently...
I agree with that, there should definitely be an error printed. It should say something like 'Failed to load splash screen (err=-4)' or something like that. The error number should provide some clues and people can dig in.
This patch would add around 200 bytes if debug() was changed to printf(). Multiply that by several hundred if we did that sort of thing throughout U-Boot.
I think that debug() is a good thing to use, but we shouldn't be obsessed with it.
Fair enough, I don't want to get obsessed about it either! But general code-size bloat is a real problem. so I think in general we should make sure an error is printed when one occurs, and only cover some common cases where the user can actually fix it (e.g SD card missing) with more detailed error messages.
So long as the error is reported (even if it is not a very specific error), people can add DEBUG and track it down.
That depends who 'people' are? Devs? Users?
Well in this case the user will never see the problem, unless someone has screwed up the splash screen image. Mostly I'm talking about devs.
Better would be to have an expanded debug() system which lets you turn debugging on globally when hunting for a problem. That would be a nice project for someone...
Regards, Simon
On 12/13/16 22:29, Simon Glass wrote:
Hi Igor,
On 12 December 2016 at 01:37, Igor Grinberg grinberg@compulab.co.il wrote:
On 12/11/16 22:27, Simon Glass wrote:
Hi Igor,
On 11 December 2016 at 10:37, Igor Grinberg grinberg@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@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).
Unless, there a kind of corruption or a user mistake and then that same user can't even understand what happened because we do not help him with an error message. You cannot know that these error messages will never happen... This is a generic code which can be used by a wide variety of platforms - I don't think you can foresee all the possible use cases.
We are talking about several tens of bytes vs. usability. If there is an error, it should be stated as such. It should not just exit silently...
I agree with that, there should definitely be an error printed. It should say something like 'Failed to load splash screen (err=-4)' or something like that. The error number should provide some clues and people can dig in.
Great idea!
This patch would add around 200 bytes if debug() was changed to printf(). Multiply that by several hundred if we did that sort of thing throughout U-Boot.
Well, I thought about using printk only on a half of the above messages and that gives about ~90 bytes, while it also can be optimized by using the same parameterized string and thus add only ~50 bytes...
I think that debug() is a good thing to use, but we shouldn't be obsessed with it.
Fair enough, I don't want to get obsessed about it either! But general code-size bloat is a real problem. so I think in general we should make sure an error is printed when one occurs, and only cover some common cases where the user can actually fix it (e.g SD card missing) with more detailed error messages.
So long as the error is reported (even if it is not a very specific error), people can add DEBUG and track it down.
That depends who 'people' are? Devs? Users?
Well in this case the user will never see the problem, unless someone has screwed up the splash screen image. Mostly I'm talking about devs.
Better would be to have an expanded debug() system which lets you turn debugging on globally when hunting for a problem. That would be a nice project for someone...
Yes, indeed that sounds like a nice project. It would be great to be able to specify the debug verbosity on per build basis (e.g. Kconfig).
Hi Simon, Igor,
On 12/14/2016 02:53 PM, Igor Grinberg wrote:
On 12/13/16 22:29, Simon Glass wrote:
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).
Unless, there a kind of corruption or a user mistake and then that same user can't even understand what happened because we do not help him with an error message. You cannot know that these error messages will never happen... This is a generic code which can be used by a wide variety of platforms - I don't think you can foresee all the possible use cases.
We are talking about several tens of bytes vs. usability. If there is an error, it should be stated as such. It should not just exit silently...
I agree with that, there should definitely be an error printed. It should say something like 'Failed to load splash screen (err=-4)' or something like that. The error number should provide some clues and people can dig in.
Great idea!
splash_load_fit() currently fails silently but still reports the error in the return value. And this function is used so that board.c calls splash_source_load()->splash_load_fit(). The board function call will get notified of the error value as provided by the return value for splash_load_fit(). In our board implementation that is actually exactly how it is done, the board function call checks the return value and prints ("Failed to load splash screen image, error: %d\n", ret) in case there was and error.
IMHO this is quite good behaviour as is, leaving it up to the implementation in the board.c if there should be a error message or not (and if it should bloat the code with another printf or not).
So long as the error is reported (even if it is not a very specific error), people can add DEBUG and track it down.
That depends who 'people' are? Devs? Users?
Well in this case the user will never see the problem, unless someone has screwed up the splash screen image. Mostly I'm talking about devs.
Better would be to have an expanded debug() system which lets you turn debugging on globally when hunting for a problem. That would be a nice project for someone...
Yes, indeed that sounds like a nice project. It would be great to be able to specify the debug verbosity on per build basis (e.g. Kconfig).
Indeed, that would be a great feature.
Regards, Tomas
Hi Tomas,
On 12/14/16 16:23, Tomas Melin wrote:
Hi Simon, Igor,
On 12/14/2016 02:53 PM, Igor Grinberg wrote:
On 12/13/16 22:29, Simon Glass wrote:
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).
Unless, there a kind of corruption or a user mistake and then that same user can't even understand what happened because we do not help him with an error message. You cannot know that these error messages will never happen... This is a generic code which can be used by a wide variety of platforms - I don't think you can foresee all the possible use cases.
We are talking about several tens of bytes vs. usability. If there is an error, it should be stated as such. It should not just exit silently...
I agree with that, there should definitely be an error printed. It should say something like 'Failed to load splash screen (err=-4)' or something like that. The error number should provide some clues and people can dig in.
Great idea!
splash_load_fit() currently fails silently but still reports the error in the return value. And this function is used so that board.c calls splash_source_load()->splash_load_fit(). The board function call will get notified of the error value as provided by the return value for splash_load_fit(). In our board implementation that is actually exactly how it is done, the board function call checks the return value and prints ("Failed to load splash screen image, error: %d\n", ret) in case there was and error.
IMHO this is quite good behaviour as is, leaving it up to the implementation in the board.c if there should be a error message or not (and if it should bloat the code with another printf or not).
Well, yes this makes sense if you care to do the work in the board code. Although, I would expect that sometime this code will be called from a generic board independent place (e.g. init array, etc.).
So long as the error is reported (even if it is not a very specific error), people can add DEBUG and track it down.
That depends who 'people' are? Devs? Users?
Well in this case the user will never see the problem, unless someone has screwed up the splash screen image. Mostly I'm talking about devs.
Better would be to have an expanded debug() system which lets you turn debugging on globally when hunting for a problem. That would be a nice project for someone...
Yes, indeed that sounds like a nice project. It would be great to be able to specify the debug verbosity on per build basis (e.g. Kconfig).
Indeed, that would be a great feature.
Regards, Tomas
Hi Igor, Simon,
On 12/15/2016 11:08 AM, Igor Grinberg wrote:
Hi Tomas,
On 12/14/16 16:23, Tomas Melin wrote:
Hi Simon, Igor,
On 12/14/2016 02:53 PM, Igor Grinberg wrote:
On 12/13/16 22:29, Simon Glass wrote:
> > 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).
Unless, there a kind of corruption or a user mistake and then that same user can't even understand what happened because we do not help him with an error message. You cannot know that these error messages will never happen... This is a generic code which can be used by a wide variety of platforms - I don't think you can foresee all the possible use cases.
We are talking about several tens of bytes vs. usability. If there is an error, it should be stated as such. It should not just exit silently...
I agree with that, there should definitely be an error printed. It should say something like 'Failed to load splash screen (err=-4)' or something like that. The error number should provide some clues and people can dig in.
Great idea!
splash_load_fit() currently fails silently but still reports the error in the return value. And this function is used so that board.c calls splash_source_load()->splash_load_fit(). The board function call will get notified of the error value as provided by the return value for splash_load_fit(). In our board implementation that is actually exactly how it is done, the board function call checks the return value and prints ("Failed to load splash screen image, error: %d\n", ret) in case there was and error.
IMHO this is quite good behaviour as is, leaving it up to the implementation in the board.c if there should be a error message or not (and if it should bloat the code with another printf or not).
Well, yes this makes sense if you care to do the work in the board code. Although, I would expect that sometime this code will be called from a generic board independent place (e.g. init array, etc.).
I don't have a strong opinion, even if I do prefer the current version. Please let me know if patch this still needs changes.
BR, Tomas
Hi Tomas,
Sorry for the late response...
On 12/20/16 07:29, Tomas Melin wrote:
Hi Igor, Simon,
On 12/15/2016 11:08 AM, Igor Grinberg wrote:
Hi Tomas,
On 12/14/16 16:23, Tomas Melin wrote:
Hi Simon, Igor,
On 12/14/2016 02:53 PM, Igor Grinberg wrote:
On 12/13/16 22:29, Simon Glass wrote:
>> >> 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).
Unless, there a kind of corruption or a user mistake and then that same user can't even understand what happened because we do not help him with an error message. You cannot know that these error messages will never happen... This is a generic code which can be used by a wide variety of platforms - I don't think you can foresee all the possible use cases.
We are talking about several tens of bytes vs. usability. If there is an error, it should be stated as such. It should not just exit silently...
I agree with that, there should definitely be an error printed. It should say something like 'Failed to load splash screen (err=-4)' or something like that. The error number should provide some clues and people can dig in.
Great idea!
splash_load_fit() currently fails silently but still reports the error in the return value. And this function is used so that board.c calls splash_source_load()->splash_load_fit(). The board function call will get notified of the error value as provided by the return value for splash_load_fit(). In our board implementation that is actually exactly how it is done, the board function call checks the return value and prints ("Failed to load splash screen image, error: %d\n", ret) in case there was and error.
IMHO this is quite good behaviour as is, leaving it up to the implementation in the board.c if there should be a error message or not (and if it should bloat the code with another printf or not).
Well, yes this makes sense if you care to do the work in the board code. Although, I would expect that sometime this code will be called from a generic board independent place (e.g. init array, etc.).
I don't have a strong opinion, even if I do prefer the current version. Please let me know if patch this still needs changes.
Well, I can see two courses this patch can take: 1) We merge it as-is and any additional board uses the splash_load_fit() will copy/paste the error handling from your board, or generalizes it and patches both boards. 2) We make a more generic approach (e.g. print error messages in the common code) and any new user (board or common call) will not need to handle the errors, but just use the call.
Either one is good with me.
Have a great holidays everyone!
Hi Igor, Simon,
On 12/26/2016 09:24 AM, Igor Grinberg wrote:
Hi Tomas,
Sorry for the late response...
On 12/20/16 07:29, Tomas Melin wrote:
Hi Igor, Simon,
On 12/15/2016 11:08 AM, Igor Grinberg wrote:
Hi Tomas,
On 12/14/16 16:23, Tomas Melin wrote:
Hi Simon, Igor,
On 12/14/2016 02:53 PM, Igor Grinberg wrote:
On 12/13/16 22:29, Simon Glass wrote:
>>> >>> 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). > > Unless, there a kind of corruption or a user mistake and then that same > user can't even understand what happened because we do not help him with > an error message. > You cannot know that these error messages will never happen... > This is a generic code which can be used by a wide variety of platforms - > I don't think you can foresee all the possible use cases. > > We are talking about several tens of bytes vs. usability. > If there is an error, it should be stated as such. It should not just > exit silently...
I agree with that, there should definitely be an error printed. It should say something like 'Failed to load splash screen (err=-4)' or something like that. The error number should provide some clues and people can dig in.
Great idea!
splash_load_fit() currently fails silently but still reports the error in the return value. And this function is used so that board.c calls splash_source_load()->splash_load_fit(). The board function call will get notified of the error value as provided by the return value for splash_load_fit(). In our board implementation that is actually exactly how it is done, the board function call checks the return value and prints ("Failed to load splash screen image, error: %d\n", ret) in case there was and error.
IMHO this is quite good behaviour as is, leaving it up to the implementation in the board.c if there should be a error message or not (and if it should bloat the code with another printf or not).
Well, yes this makes sense if you care to do the work in the board code. Although, I would expect that sometime this code will be called from a generic board independent place (e.g. init array, etc.).
I don't have a strong opinion, even if I do prefer the current version. Please let me know if patch this still needs changes.
Well, I can see two courses this patch can take:
- We merge it as-is and any additional board uses the splash_load_fit() will copy/paste the error handling from your board, or generalizes it and patches both boards.
- We make a more generic approach (e.g. print error messages in the common code) and any new user (board or common call) will not need to handle the errors, but just use the call.
Either one is good with me.
I'll update the patch to use printf:s for the latter statements (as discussed above.), I will send out a new version shortly.
Thanks, Tomas
Have a great holidays everyone!
On 5 December 2016 at 02:36, Tomas Melin tomas.melin@vaisala.com wrote:
Sort include files in accordance to u-boot coding style.
Signed-off-by: Tomas Melin tomas.melin@vaisala.com
Changes in v4:
- Added missing changelog
Changes in v3:
- Change patch order so that include sort patch comes prior to adding new include
Changes in v2:
- Add separate patch for sorting include files
common/splash_source.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Note: It is 'U-Boot', not 'u-boot'
participants (3)
-
Igor Grinberg -
Simon Glass -
Tomas Melin