[U-Boot] [PATCH v4 1/2] splash: sort include files

[U-Boot] [PATCH v4 1/2] splash: sort include files

[Tomas Melin]

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;
 
-- 
2.1.4

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

[Tomas Melin]

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 {
-- 
2.1.4

[U-Boot] [PATCH v4 1/2] splash: sort include files

[Simon Glass]

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'

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

[Simon Glass]

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>

[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@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.

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

[Simon Glass]

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

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

[Igor Grinberg]

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?


-- 
Regards,
Igor.

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

[Simon Glass]

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

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

[Igor Grinberg]

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


-- 
Regards,
Igor.

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

[Tomas Melin]

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

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

[Igor Grinberg]

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
> 

-- 
Regards,
Igor.

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

[Tomas Melin]

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

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

[Igor Grinberg]

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!

-- 
Regards,
Igor.

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

[Tomas Melin]

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

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!
>