[U-Boot] [PATCH v2 0/2] Load splash from FIT image (internal, external, offset)

[U-Boot] [PATCH v2 0/2] Load splash from FIT image (internal, external, offset)

[Mark Jonas]

We store a splash screen in SPI-NOR. We chose to use a FIT image as a
container because we want to
  - store more than just the splash screen in SPI-NOR,
  - do not create a bunch of MTD partitions,
  - do not waste storage space, and
  - avoid the overhead of a real file system.

In general U-Boot already supports this. But there were some ripples we
had to smooth out. The following patches resolve these.

Changes since v1:
  - Document default name in README.splashprepare
  - Use ALIGN() macro

Leo Ruan (2):
  splash: Use splashfile instead of location->name
  splash: Load internal and external data from FIT

 common/splash_source.c   | 65 +++++++++++++++++++++++++++++++-----------------
 doc/README.splashprepare |  9 ++++---
 2 files changed, 48 insertions(+), 26 deletions(-)

-- 
2.7.4

[U-Boot] [PATCH v2 1/2] splash: Use splashfile instead of location->name

[Mark Jonas]

From: Leo Ruan <tingquan.ruan@cn.bosch.com>

The splash image could be loaded from different sources (e.g. sf, mmc)
with different formats (e.g. raw, file-system). These sources are
structured by a board dependent object 'splash_location'. To decide
where is the splash image loaded, following environment variables are
used to select the splash source and file:
- 'splashsource' is used to select the splash source by setting its
  value to specified name of splash location.
- 'splashfile' specify the name of splash image file

But, when loads the splash image from FIT, the name of splash image
within FIT is specified by splash location name. Due to the splash
location name is already used for the splash source, its name may
conflicts with the name of splash image.

To solve the conflict, the environment variable 'splashfile' is used
to specify the splash image in FIT, and keeps the splash location
name for the splash source.

Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/splash_source.c   | 10 ++++++++--
 doc/README.splashprepare |  9 ++++++---
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/common/splash_source.c b/common/splash_source.c
index 62763b9..e1e73db 100644
--- a/common/splash_source.c
+++ b/common/splash_source.c
@@ -303,6 +303,7 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
 {
 	int res;
 	int node_offset;
+	const char *splash_file;
 	int splash_offset;
 	int splash_size;
 	struct image_header *img_header;
@@ -335,10 +336,15 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
 		return -EINVAL;
 	}
 
-	node_offset = fit_image_get_node(fit_header, location->name);
+	/* Get the splash image node */
+	splash_file = env_get("splashfile");
+	if (!splash_file)
+		splash_file = SPLASH_SOURCE_DEFAULT_FILE_NAME;
+
+	node_offset = fit_image_get_node(fit_header, splash_file);
 	if (node_offset < 0) {
 		debug("Could not find splash image '%s' in FIT\n",
-		      location->name);
+		      splash_file);
 		return -ENOENT;
 	}
 
diff --git a/doc/README.splashprepare b/doc/README.splashprepare
index f1418de..3cb5b5a 100644
--- a/doc/README.splashprepare
+++ b/doc/README.splashprepare
@@ -26,6 +26,9 @@ 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.
+enabled. The FIT image has to start at the 'offset' field address in the
+selected splash location. The name of splash image within the FIT shall be
+specified by the environment variable "splashfile".
+
+In case the environment variable "splashfile" is not defined the default name
+'splash.bmp' will be used.
-- 
2.7.4

[U-Boot] [PATCH v2 2/2] splash: Load internal and external data from FIT

[Mark Jonas]

From: Leo Ruan <tingquan.ruan@cn.bosch.com>

The FIT image could contain the splash data in 3 different structure:
- The splash data is embedded in FIT image (internal)
  In this case, the property 'data' presents in FIT image header. And
  internal information 'start' and 'end' represent the location and
  size of splash data inside of FIT image.
- The splash data is external with absolute position in FIT image
  This case is made by 'mkimage -p [pos]'. The splash data is stored
  at the absolute position. Instead the property 'data', the properties
  'data-position' and 'data-size' are used to specify the location and
  size of the splash data.
- the splash data is external with relative offset in FIT image
  This case is made by 'mkimage -E'. The splash data is placed after
  the FIT image header with 4 byte alignment. Instead the property
  'data', the properties 'data-offset' and 'data-size' are used to
  specify the location and size of the splash data.

Currently, the splash only support to load external data with relative
offset from FIT image. This commit make it possible to load the splash
data embedded in FIT image or the external data with absolute position

This inspiration comes from Simon Glass <sjg@chromium.org>, see
common/spl_fit.c

Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/splash_source.c | 55 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/common/splash_source.c b/common/splash_source.c
index e1e73db..8f276a3 100644
--- a/common/splash_source.c
+++ b/common/splash_source.c
@@ -304,8 +304,11 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
 	int res;
 	int node_offset;
 	const char *splash_file;
-	int splash_offset;
-	int splash_size;
+	const void *internal_splash_data;
+	size_t internal_splash_size;
+	int external_splash_addr;
+	int external_splash_size;
+	bool is_splash_external = false;
 	struct image_header *img_header;
 	const u32 *fit_header;
 	u32 fit_size;
@@ -348,29 +351,39 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
 		return -ENOENT;
 	}
 
-	res = fit_image_get_data_offset(fit_header, node_offset,
-					&splash_offset);
-	if (res < 0) {
-		printf("Failed to load splash image (err=%d)\n", res);
-		return res;
+	/* Extract the splash data from FIT */
+	/* 1. Test if splash is in FIT internal data. */
+	if (!fit_image_get_data(fit_header, node_offset, &internal_splash_data, &internal_splash_size))
+		memmove((void *)bmp_load_addr, internal_splash_data, internal_splash_size);
+	/* 2. Test if splash is in FIT external data with fixed position. */
+	else if (!fit_image_get_data_position(fit_header, node_offset, &external_splash_addr))
+		is_splash_external = true;
+	/* 3. Test if splash is in FIT external data with offset. */
+	else if (!fit_image_get_data_offset(fit_header, node_offset, &external_splash_addr)) {
+		/* Align data offset to 4-byte boundary */
+		fit_size = ALIGN(fdt_totalsize(fit_header), 4);
+		/* External splash offset means the offset by end of FIT header */
+		external_splash_addr += location->offset + fit_size;
+		is_splash_external = true;
+	} else {
+		printf("Failed to get splash image from FIT\n");
+		return -ENODATA;
 	}
 
-	res = fit_image_get_data_size(fit_header, node_offset, &splash_size);
-	if (res < 0) {
-		printf("Failed to load splash image (err=%d)\n", res);
-		return res;
+	if (is_splash_external) {
+		res = fit_image_get_data_size(fit_header, node_offset, &external_splash_size);
+		if (res < 0) {
+			printf("Failed to get size of splash image (err=%d)\n", res);
+			return res;
+		}
+
+		/* Read in the splash data */
+		location->offset = external_splash_addr;
+		res = splash_storage_read_raw(location, bmp_load_addr, external_splash_size);
+		if (res < 0)
+			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 */
-- 
2.7.4

[U-Boot] [PATCH v2 1/2] splash: Use splashfile instead of location->name

[Stefano Babic]

On 05/02/19 16:29, Mark Jonas wrote:
> From: Leo Ruan <tingquan.ruan@cn.bosch.com>
> 
> The splash image could be loaded from different sources (e.g. sf, mmc)
> with different formats (e.g. raw, file-system). These sources are
> structured by a board dependent object 'splash_location'. To decide
> where is the splash image loaded, following environment variables are
> used to select the splash source and file:
> - 'splashsource' is used to select the splash source by setting its
>   value to specified name of splash location.
> - 'splashfile' specify the name of splash image file
> 
> But, when loads the splash image from FIT, the name of splash image
> within FIT is specified by splash location name. Due to the splash
> location name is already used for the splash source, its name may
> conflicts with the name of splash image.
> 
> To solve the conflict, the environment variable 'splashfile' is used
> to specify the splash image in FIT, and keeps the splash location
> name for the splash source.
> 
> Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  common/splash_source.c   | 10 ++++++++--
>  doc/README.splashprepare |  9 ++++++---
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/common/splash_source.c b/common/splash_source.c
> index 62763b9..e1e73db 100644
> --- a/common/splash_source.c
> +++ b/common/splash_source.c
> @@ -303,6 +303,7 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
>  {
>  	int res;
>  	int node_offset;
> +	const char *splash_file;
>  	int splash_offset;
>  	int splash_size;
>  	struct image_header *img_header;
> @@ -335,10 +336,15 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
>  		return -EINVAL;
>  	}
>  
> -	node_offset = fit_image_get_node(fit_header, location->name);
> +	/* Get the splash image node */
> +	splash_file = env_get("splashfile");
> +	if (!splash_file)
> +		splash_file = SPLASH_SOURCE_DEFAULT_FILE_NAME;
> +
> +	node_offset = fit_image_get_node(fit_header, splash_file);
>  	if (node_offset < 0) {
>  		debug("Could not find splash image '%s' in FIT\n",
> -		      location->name);
> +		      splash_file);
>  		return -ENOENT;
>  	}
>  
> diff --git a/doc/README.splashprepare b/doc/README.splashprepare
> index f1418de..3cb5b5a 100644
> --- a/doc/README.splashprepare
> +++ b/doc/README.splashprepare
> @@ -26,6 +26,9 @@ 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.
> +enabled. The FIT image has to start at the 'offset' field address in the
> +selected splash location. The name of splash image within the FIT shall be
> +specified by the environment variable "splashfile".
> +
> +In case the environment variable "splashfile" is not defined the default name
> +'splash.bmp' will be used.
> 

Reviewed-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v2 2/2] splash: Load internal and external data from FIT

[Stefano Babic]

On 05/02/19 16:29, Mark Jonas wrote:
> From: Leo Ruan <tingquan.ruan@cn.bosch.com>
> 
> The FIT image could contain the splash data in 3 different structure:
> - The splash data is embedded in FIT image (internal)
>   In this case, the property 'data' presents in FIT image header. And
>   internal information 'start' and 'end' represent the location and
>   size of splash data inside of FIT image.
> - The splash data is external with absolute position in FIT image
>   This case is made by 'mkimage -p [pos]'. The splash data is stored
>   at the absolute position. Instead the property 'data', the properties
>   'data-position' and 'data-size' are used to specify the location and
>   size of the splash data.
> - the splash data is external with relative offset in FIT image
>   This case is made by 'mkimage -E'. The splash data is placed after
>   the FIT image header with 4 byte alignment. Instead the property
>   'data', the properties 'data-offset' and 'data-size' are used to
>   specify the location and size of the splash data.
> 
> Currently, the splash only support to load external data with relative
> offset from FIT image. This commit make it possible to load the splash
> data embedded in FIT image or the external data with absolute position
> 
> This inspiration comes from Simon Glass <sjg@chromium.org>, see
> common/spl_fit.c
> 
> Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  common/splash_source.c | 55 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/common/splash_source.c b/common/splash_source.c
> index e1e73db..8f276a3 100644
> --- a/common/splash_source.c
> +++ b/common/splash_source.c
> @@ -304,8 +304,11 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
>  	int res;
>  	int node_offset;
>  	const char *splash_file;
> -	int splash_offset;
> -	int splash_size;
> +	const void *internal_splash_data;
> +	size_t internal_splash_size;
> +	int external_splash_addr;
> +	int external_splash_size;
> +	bool is_splash_external = false;
>  	struct image_header *img_header;
>  	const u32 *fit_header;
>  	u32 fit_size;
> @@ -348,29 +351,39 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
>  		return -ENOENT;
>  	}
>  
> -	res = fit_image_get_data_offset(fit_header, node_offset,
> -					&splash_offset);
> -	if (res < 0) {
> -		printf("Failed to load splash image (err=%d)\n", res);
> -		return res;
> +	/* Extract the splash data from FIT */
> +	/* 1. Test if splash is in FIT internal data. */
> +	if (!fit_image_get_data(fit_header, node_offset, &internal_splash_data, &internal_splash_size))
> +		memmove((void *)bmp_load_addr, internal_splash_data, internal_splash_size);
> +	/* 2. Test if splash is in FIT external data with fixed position. */
> +	else if (!fit_image_get_data_position(fit_header, node_offset, &external_splash_addr))
> +		is_splash_external = true;
> +	/* 3. Test if splash is in FIT external data with offset. */
> +	else if (!fit_image_get_data_offset(fit_header, node_offset, &external_splash_addr)) {
> +		/* Align data offset to 4-byte boundary */
> +		fit_size = ALIGN(fdt_totalsize(fit_header), 4);
> +		/* External splash offset means the offset by end of FIT header */
> +		external_splash_addr += location->offset + fit_size;
> +		is_splash_external = true;
> +	} else {
> +		printf("Failed to get splash image from FIT\n");
> +		return -ENODATA;
>  	}
>  
> -	res = fit_image_get_data_size(fit_header, node_offset, &splash_size);
> -	if (res < 0) {
> -		printf("Failed to load splash image (err=%d)\n", res);
> -		return res;
> +	if (is_splash_external) {
> +		res = fit_image_get_data_size(fit_header, node_offset, &external_splash_size);
> +		if (res < 0) {
> +			printf("Failed to get size of splash image (err=%d)\n", res);
> +			return res;
> +		}
> +
> +		/* Read in the splash data */
> +		location->offset = external_splash_addr;
> +		res = splash_storage_read_raw(location, bmp_load_addr, external_splash_size);
> +		if (res < 0)
> +			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 */
> 


Reviewed-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v2 1/2] splash: Use splashfile instead of location->name

[Melin Tomas]

Hi,

On 2/5/19 5:29 PM, Mark Jonas wrote:
> From: Leo Ruan <tingquan.ruan@cn.bosch.com>
>
>
>   common/splash_source.c   | 10 ++++++++--
>   doc/README.splashprepare |  9 ++++++---
>   2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/common/splash_source.c b/common/splash_source.c
> index 62763b9..e1e73db 100644
> --- a/common/splash_source.c
> +++ b/common/splash_source.c
> @@ -303,6 +303,7 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
>   {
>   	int res;
>   	int node_offset;
> +	const char *splash_file;
>   	int splash_offset;
>   	int splash_size;
>   	struct image_header *img_header;
> @@ -335,10 +336,15 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
>   		return -EINVAL;
>   	}
>   
> -	node_offset = fit_image_get_node(fit_header, location->name);
> +	/* Get the splash image node */
> +	splash_file = env_get("splashfile");
> +	if (!splash_file)
> +		splash_file = SPLASH_SOURCE_DEFAULT_FILE_NAME;
> +
> +	node_offset = fit_image_get_node(fit_header, splash_file);

It looks like this will break boards relying on existing logic with 
location->name (3 boards upstream).

Could fallback here instead be "location->name" as before, keeping 
compability with the current implementation? I.e.

+	if (!splash_file)
+		splash_file = location->name;

BR,

Tomas

[U-Boot] [PATCH v2 1/2] splash: Use splashfile instead of location->name

[Jonas Mark]

Hi Tomas,

> > +	/* Get the splash image node */
> > +	splash_file = env_get("splashfile");
> > +	if (!splash_file)
> > +		splash_file = SPLASH_SOURCE_DEFAULT_FILE_NAME;
> > +
> > +	node_offset = fit_image_get_node(fit_header, splash_file);
> 
> It looks like this will break boards relying on existing logic with
> location->name (3 boards upstream).
> 
> Could fallback here instead be "location->name" as before, keeping
> compability with the current implementation? I.e.
> 
> +	if (!splash_file)
> +		splash_file = location->name;

Can you point me to the 3 upstream boards which are relying on this?

Is it compulab/cm_fx6, compulab/cm_t35, and menlo/m53menlo?

compulab/cm_fx6 uses .name = "nand" and SPLASH_STORAGE_RAW. Thus it is
not be affected by changes in splash_load_fit().

http://git.denx.de/?p=u-boot.git;a=blob;f=board/compulab/cm_t35/cm_t35.c;h=4d171f4900be8eb6deea730435825b3edd805612;hb=HEAD#l63

compulab/cm_fx6 uses four different .name and splash locations. They are
either SPLASH_STORAGE_RAW or SPLASH_STORAGE_FS. Thus it is not affected.

http://git.denx.de/?p=u-boot.git;a=blob;f=board/compulab/cm_fx6/cm_fx6.c;h=d42f57d4b7014b3bd6550af186504f2c580585d2;hb=HEAD#l43

menlo/m53menlo uses .name = "mmc_fs" and SPLASH_STORAGE_FS. Thus it is
not affected.

http://git.denx.de/?p=u-boot.git;a=blob;f=board/menlo/m53menlo/m53menlo.c;h=6bdd6d5b234dd0474b8288abda32b8099a570348;hb=HEAD#l336

I conclude that no upstream board is actually affected. They would only be affecte if they would use SPLASH_STORAGE_FIT.

http://git.denx.de/?p=u-boot.git;a=blob;f=common/splash_source.c;h=62763b9ebd5698b454280a6d4b62cd23a7312786;hb=HEAD#l409

Regards,
Mark


Building Technologies, Panel Software Fire (BT-FIR/ENG1) 
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com

Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118 
Aufsichtsratsvorsitzender: Christian Fischer; Geschäftsführung: Tanja Rückert, Andreas Bartz, Thomas Quante, Bernhard Schuster

[U-Boot] [PATCH v2 1/2] splash: Use splashfile instead of location->name

[Melin Tomas]

Hi Mark,

>> Could fallback here instead be "location->name" as before, keeping
>> compability with the current implementation? I.e.
>>
>> +	if (!splash_file)
>> +		splash_file = location->name;
> Can you point me to the 3 upstream boards which are relying on this?
>
> Is it compulab/cm_fx6, compulab/cm_t35, and menlo/m53menlo?
>
> compulab/cm_fx6 uses .name = "nand" and SPLASH_STORAGE_RAW. Thus it is
> not be affected by changes in splash_load_fit().
>
> http://git.denx.de/?p=u-boot.git;a=blob;f=board/compulab/cm_t35/cm_t35.c;h=4d171f4900be8eb6deea730435825b3edd805612;hb=HEAD#l63
>
> compulab/cm_fx6 uses four different .name and splash locations. They are
> either SPLASH_STORAGE_RAW or SPLASH_STORAGE_FS. Thus it is not affected.
>
> http://git.denx.de/?p=u-boot.git;a=blob;f=board/compulab/cm_fx6/cm_fx6.c;h=d42f57d4b7014b3bd6550af186504f2c580585d2;hb=HEAD#l43
>
> menlo/m53menlo uses .name = "mmc_fs" and SPLASH_STORAGE_FS. Thus it is
> not affected.
>
> http://git.denx.de/?p=u-boot.git;a=blob;f=board/menlo/m53menlo/m53menlo.c;h=6bdd6d5b234dd0474b8288abda32b8099a570348;hb=HEAD#l336
>
> I conclude that no upstream board is actually affected. They would only be affecte if they would use SPLASH_STORAGE_FIT.

You are right, indeed upstream boards should be ok. However, some other 
non upstream boards I know of would be affected.

So my initial question still stands, could existing implementation be 
kept as fallback?

+	if (!splash_file)
+		splash_file = location->name;

location->name has worked fine for those cases, so it would be nice to 
keep that option intact.

BR,

Tomas


>
> http://git.denx.de/?p=u-boot.git;a=blob;f=common/splash_source.c;h=62763b9ebd5698b454280a6d4b62cd23a7312786;hb=HEAD#l409
>
> Regards,
> Mark
>
>
> Building Technologies, Panel Software Fire (BT-FIR/ENG1)
> Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com
>
> Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118
> Aufsichtsratsvorsitzender: Christian Fischer; Geschäftsführung: Tanja Rückert, Andreas Bartz, Thomas Quante, Bernhard Schuster

[U-Boot] [PATCH v2 1/2] splash: Use splashfile instead of location->name

[Stefano Babic]

Hi Thomas,

On 06/02/19 14:44, Melin Tomas wrote:
> Hi Mark,
> 
>>> Could fallback here instead be "location->name" as before, keeping
>>> compability with the current implementation? I.e.
>>>
>>> +	if (!splash_file)
>>> +		splash_file = location->name;
>> Can you point me to the 3 upstream boards which are relying on this?
>>
>> Is it compulab/cm_fx6, compulab/cm_t35, and menlo/m53menlo?
>>
>> compulab/cm_fx6 uses .name = "nand" and SPLASH_STORAGE_RAW. Thus it is
>> not be affected by changes in splash_load_fit().
>>
>> http://git.denx.de/?p=u-boot.git;a=blob;f=board/compulab/cm_t35/cm_t35.c;h=4d171f4900be8eb6deea730435825b3edd805612;hb=HEAD#l63
>>
>> compulab/cm_fx6 uses four different .name and splash locations. They are
>> either SPLASH_STORAGE_RAW or SPLASH_STORAGE_FS. Thus it is not affected.
>>
>> http://git.denx.de/?p=u-boot.git;a=blob;f=board/compulab/cm_fx6/cm_fx6.c;h=d42f57d4b7014b3bd6550af186504f2c580585d2;hb=HEAD#l43
>>
>> menlo/m53menlo uses .name = "mmc_fs" and SPLASH_STORAGE_FS. Thus it is
>> not affected.
>>
>> http://git.denx.de/?p=u-boot.git;a=blob;f=board/menlo/m53menlo/m53menlo.c;h=6bdd6d5b234dd0474b8288abda32b8099a570348;hb=HEAD#l336
>>
>> I conclude that no upstream board is actually affected. They would only be affecte if they would use SPLASH_STORAGE_FIT.
> 
> You are right, indeed upstream boards should be ok. However, some other 
> non upstream boards I know of would be affected.
> 

Well, to be honest: we cannot care in mainline about non upstream
boards. They are not tested at all and they can go broken at any time.

If people / companies want that their board are efficeintly maintained,
they have to take the path to work for including boards into official
tree, and maintain them.

> So my initial question still stands, could existing implementation be 
> kept as fallback?
> 
> +	if (!splash_file)
> +		splash_file = location->name;
> 
> location->name has worked fine for those cases, so it would be nice to 
> keep that option intact.

Best regards,
Stefano Babic

> 
> BR,
> 
> Tomas
> 
> 
>>
>> http://git.denx.de/?p=u-boot.git;a=blob;f=common/splash_source.c;h=62763b9ebd5698b454280a6d4b62cd23a7312786;hb=HEAD#l409
>>
>> Regards,
>> Mark
>>
>>
>> Building Technologies, Panel Software Fire (BT-FIR/ENG1)
>> Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com
>>
>> Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118
>> Aufsichtsratsvorsitzender: Christian Fischer; Geschäftsführung: Tanja Rückert, Andreas Bartz, Thomas Quante, Bernhard Schuster


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v2 1/2] splash: Use splashfile instead of location->name

[Melin Tomas]

On 2/5/19 5:29 PM, Mark Jonas wrote:
> From: Leo Ruan <tingquan.ruan@cn.bosch.com>
>
> The splash image could be loaded from different sources (e.g. sf, mmc)
> with different formats (e.g. raw, file-system). These sources are
> structured by a board dependent object 'splash_location'. To decide
> where is the splash image loaded, following environment variables are
> used to select the splash source and file:
> - 'splashsource' is used to select the splash source by setting its
>    value to specified name of splash location.
> - 'splashfile' specify the name of splash image file
>
> But, when loads the splash image from FIT, the name of splash image
> within FIT is specified by splash location name. Due to the splash
> location name is already used for the splash source, its name may
> conflicts with the name of splash image.
>
> To solve the conflict, the environment variable 'splashfile' is used
> to specify the splash image in FIT, and keeps the splash location
> name for the splash source.
>
> Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>   common/splash_source.c   | 10 ++++++++--
>   doc/README.splashprepare |  9 ++++++---
>   2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/common/splash_source.c b/common/splash_source.c
> index 62763b9..e1e73db 100644
> --- a/common/splash_source.c
> +++ b/common/splash_source.c
> @@ -303,6 +303,7 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
>   {
>   	int res;
>   	int node_offset;
> +	const char *splash_file;
>   	int splash_offset;
>   	int splash_size;
>   	struct image_header *img_header;
> @@ -335,10 +336,15 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
>   		return -EINVAL;
>   	}
>   
> -	node_offset = fit_image_get_node(fit_header, location->name);
> +	/* Get the splash image node */
> +	splash_file = env_get("splashfile");
> +	if (!splash_file)
> +		splash_file = SPLASH_SOURCE_DEFAULT_FILE_NAME;
> +
> +	node_offset = fit_image_get_node(fit_header, splash_file);
>   	if (node_offset < 0) {
>   		debug("Could not find splash image '%s' in FIT\n",
> -		      location->name);
> +		      splash_file);
>   		return -ENOENT;
>   	}
>   
> diff --git a/doc/README.splashprepare b/doc/README.splashprepare
> index f1418de..3cb5b5a 100644
> --- a/doc/README.splashprepare
> +++ b/doc/README.splashprepare
> @@ -26,6 +26,9 @@ 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.
> +enabled. The FIT image has to start at the 'offset' field address in the
> +selected splash location. The name of splash image within the FIT shall be
> +specified by the environment variable "splashfile".
> +
> +In case the environment variable "splashfile" is not defined the default name
> +'splash.bmp' will be used

Reviewed-by: Tomas Melin <tomas.melin@vaisala.com>



Regards,
Tomas Melin

[U-Boot] [PATCH v2 1/2] splash: Use splashfile instead of location->name

[Melin Tomas]

Hi Mark,

On 2/6/19 5:42 PM, Jonas Mark (BT-FIR/ENG1) wrote:
>> If I understand the original issue you had correctly, could it also have
>> been fixed by renaming splash node in FIT image?
> Yes, that's we actually started with. But once we started adding more
> files to that FIT image we saw the problem: Then we had files in the FIT
> image with "nice" names and one "ugly duckling". Now it is fixed.

The same end result could be achieved by selecting an appropriate name 
for the 'splash_location' 'name' field, sounds like no need for new env 
variable.

But please fill in if there is something more to the original problem

>>> Why do I think so? Because the behavior is now consistent to what
>>> splash_load_fs() does. That is, if there is an environment variable
>>> "splashfile" the splash screen name is taken from there. Else, the
>>> fixed default name "splash.bmp" is used.

Except there is an important difference. This is loading a node from a 
FIT image, not a file from a file system.

The patch sets the default _node_ name to look for in the FIT to be 
"splash.bmp", that's a rather odd name for a FIT node.

>> Symmetry is nice, so ok. But still, it now rules out the possiblity to use the
>> splash name in the FIT without defining additional "splashfile"
>> to the env.
> This is not correct. In case you do not define "splashfile" in the
> environment the consistent (!) default name is "splash.bmp".

No, that's not what I meant. Prior to the change, splash node name in 
FIT could be selected freely

without adding "splashfile" to environment. That's not longer possible.

BR,

Tomas

[U-Boot] [PATCH v2 1/2] splash: Use splashfile instead of location->name

[Jonas Mark]

Hi Tomas,

> >> So my initial question still stands, could existing implementation be
> >> kept as fallback?
> >>
> >> +	if (!splash_file)
> >> +		splash_file = location->name;
> >>
> >> location->name has worked fine for those cases, so it would be nice
> >> location->to
> >> keep that option intact.
> > I think that the .name field of struct splash_location is actually
> > intended to hold a human readable name for the storage itself. That
> > the code so far uses the storage location name synonymously with the
> > splash screen name is unfortunate. That's why we decided to fix this.
> > I am convinced that the patch actually makes it easier for others to
> > start reading splash screens from a FIT image.
> 
> If I understand the original issue you had correctly, could it also have
> been fixed by renaming splash node in FIT image?

Yes, that's we actually started with. But once we started adding more
files to that FIT image we saw the problem: Then we had files in the FIT
image with "nice" names and one "ugly duckling". Now it is fixed.

Patch 2/2 shows more things we fixed on the way and are now
contributing.

> > Why do I think so? Because the behavior is now consistent to what
> > splash_load_fs() does. That is, if there is an environment variable
> > "splashfile" the splash screen name is taken from there. Else, the
> > fixed default name "splash.bmp" is used.
> 
> Symmetry is nice, so ok. But still, it now rules out the possiblity to use the
> splash name in the FIT without defining additional "splashfile"
> to the env.

This is not correct. In case you do not define "splashfile" in the
environment the consistent (!) default name is "splash.bmp".

Regards,
Mark


Building Technologies, Panel Software Fire (BT-FIR/ENG1) 
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com

Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118 
Aufsichtsratsvorsitzender: Christian Fischer; Geschäftsführung: Tanja Rückert, Andreas Bartz, Thomas Quante, Bernhard Schuster 

[U-Boot] [PATCH v2 1/2] splash: Use splashfile instead of location->name

[Melin Tomas]

Hi Mark,

On 2/6/19 4:30 PM, Jonas Mark (BT-FIR/ENG1) wrote:
>
>> So my initial question still stands, could existing implementation be kept
>> as fallback?
>>
>> +	if (!splash_file)
>> +		splash_file = location->name;
>>
>> location->name has worked fine for those cases, so it would be nice to
>> keep that option intact.
> I think that the .name field of struct splash_location is actually
> intended to hold a human readable name for the storage itself. That the
> code so far uses the storage location name synonymously with the splash
> screen name is unfortunate. That's why we decided to fix this. I am
> convinced that the patch actually makes it easier for others to start
> reading splash screens from a FIT image.

If I understand the original issue you had correctly, could it also have 
been fixed by renaming splash node in FIT image?

>
> Why do I think so? Because the behavior is now consistent to
> what splash_load_fs() does. That is, if there is an environment variable
> "splashfile" the splash screen name is taken from there. Else, the fixed
> default name "splash.bmp" is used.

Symmetry is nice, so ok. But still, it now rules out the possiblity to 
use the splash name in the FIT without defining additional "splashfile" 
to the env.

BR,

Tomas


>
> Regards,
> Mark
>
>
> Building Technologies, Panel Software Fire (BT-FIR/ENG1)
> Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com
>
> Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118
> Aufsichtsratsvorsitzender: Christian Fischer; Geschäftsführung: Tanja Rückert, Andreas Bartz, Thomas Quante, Bernhard Schuster

[U-Boot] [PATCH v2 1/2] splash: Use splashfile instead of location->name

[Jonas Mark]

Hi Tomas,

> > I conclude that no upstream board is actually affected. They would only
> be affecte if they would use SPLASH_STORAGE_FIT.
> 
> You are right, indeed upstream boards should be ok. However, some
> other non upstream boards I know of would be affected.
> 
> So my initial question still stands, could existing implementation be kept
> as fallback?
> 
> +	if (!splash_file)
> +		splash_file = location->name;
> 
> location->name has worked fine for those cases, so it would be nice to
> keep that option intact.

I think that the .name field of struct splash_location is actually
intended to hold a human readable name for the storage itself. That the
code so far uses the storage location name synonymously with the splash
screen name is unfortunate. That's why we decided to fix this. I am
convinced that the patch actually makes it easier for others to start
reading splash screens from a FIT image.

Why do I think so? Because the behavior is now consistent to
what splash_load_fs() does. That is, if there is an environment variable
"splashfile" the splash screen name is taken from there. Else, the fixed
default name "splash.bmp" is used.

Regards,
Mark


Building Technologies, Panel Software Fire (BT-FIR/ENG1) 
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com

Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118 
Aufsichtsratsvorsitzender: Christian Fischer; Geschäftsführung: Tanja Rückert, Andreas Bartz, Thomas Quante, Bernhard Schuster