[PATCH 0/8] spl: fit: Play nicely with OP-TEE and Linux

[PATCH 0/8] spl: fit: Play nicely with OP-TEE and Linux

[Alexandru Gagniuc]

This patch series is part of a larger effort to get linux to boot
really fast alongside a secure OS. One piece of the puzzle is getting
Linux and OP-TEE to boot straight from SPL. This is where the FIT
image comes in.

The "simple" fit code was mostly ready for this, although it was
quite difficult to navigate. As I was figuring out the required
changes, I realized I had also managed to do some major refactoring.
In fact, of the eight changes, (6) are refactoring patches, and (2)
are functional changes.

I wish I could have written this with a negative line count. I have
unfortunately failed at that, and this series will be adding 12 lines
to u-boot. The takeaway is that the following type FIT can be loaded
from SPL directly:

        images {
                optee at 1 {
                        type = "tee";
                        os = "tee";
                        ...
                };
                kernel at 1 {
                        type = "kernel";
                        os = "linux";
			...
                };
                fdt at devicetree.dtb { ... }
                fdt at bootargs { ... };
        };

        configurations {
                conf at quickboot {
                        kernel = "optee at 1";
                        fdt = "fdt at stm32mp157c-ev1.dtb", "fdt at bootargs";
                        loadables = "kernel at 1";
                };
        };


This series is designed to apply to u-boot/next:
  * 8351a29d2d Merge tag 'dm-pull-14dec20' of git://git.denx.de/u-boot-dm into next
   
Alexandru Gagniuc (8):
  spl: fit: Drop 'length' argument to board_spl_fit_post_load()
  spl: fit: Factor out FIT parsing and use a context struct
  spl: fit: Pass FIT context via a structure pointer
  spl: fit: Remove useless loop in spl_fit_get_image_name()
  spl: fit: Only look up FIT configuration node once
  image: Do not #if guard board_fit_config_name_match() prototype
  spl: fit: Replace #ifdef blocks with more readable constructs
  spl: fit: Load devicetree when a Linux payload is found

 arch/arm/mach-imx/spl.c |   5 +-
 common/spl/spl_fit.c    | 260 +++++++++++++++++++++-------------------
 include/image.h         |   3 -
 include/spl.h           |   2 +-
 4 files changed, 141 insertions(+), 129 deletions(-)

-- 
2.26.2

[PATCH 1/8] spl: fit: Drop 'length' argument to board_spl_fit_post_load()

[Alexandru Gagniuc]

The size is derived from the FIT image itself. Any alignment
requirements are machine-specific and known by the board code. Thus
the total length can be derived from the FIT image and knowledge of
the platform. The 'length' argument is redundant. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 arch/arm/mach-imx/spl.c | 5 +++--
 common/spl/spl_fit.c    | 4 ++--
 include/spl.h           | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
index aa2686bb92..11255798d3 100644
--- a/arch/arm/mach-imx/spl.c
+++ b/arch/arm/mach-imx/spl.c
@@ -18,6 +18,7 @@
 #include <asm/mach-imx/hab.h>
 #include <asm/mach-imx/boot_mode.h>
 #include <g_dnl.h>
+#include <linux/libfdt.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -318,9 +319,9 @@ ulong board_spl_fit_size_align(ulong size)
 	return size;
 }
 
-void board_spl_fit_post_load(ulong load_addr, size_t length)
+void board_spl_fit_post_load(const void *fit)
 {
-	u32 offset = length - CONFIG_CSF_SIZE;
+	u32 offset = ALIGN(fdt_totalsize(fit), 0x1000);
 
 	if (imx_hab_authenticate_image(load_addr,
 				       offset + IVT_SIZE + CSF_PAD_SIZE,
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 795e2922ce..1b4a7f6b15 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -26,7 +26,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
 #endif
 
-__weak void board_spl_fit_post_load(ulong load_addr, size_t length)
+__weak void board_spl_fit_post_load(const void *fit)
 {
 }
 
@@ -722,7 +722,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	spl_image->flags |= SPL_FIT_FOUND;
 
 #ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load((ulong)fit, size);
+	board_spl_fit_post_load(fit);
 #endif
 
 	return 0;
diff --git a/include/spl.h b/include/spl.h
index 374a295fa3..f63829a99e 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -632,7 +632,7 @@ int board_return_to_bootrom(struct spl_image_info *spl_image,
  * board_spl_fit_post_load - allow process images after loading finished
  *
  */
-void board_spl_fit_post_load(ulong load_addr, size_t length);
+void board_spl_fit_post_load(const void *fit);
 
 /**
  * board_spl_fit_size_align - specific size align before processing payload
-- 
2.26.2

[PATCH 1/8] spl: fit: Drop 'length' argument to board_spl_fit_post_load()

[Peng Fan]

> Subject: [PATCH 1/8] spl: fit: Drop 'length' argument to
> board_spl_fit_post_load()
> 
> The size is derived from the FIT image itself. Any alignment requirements are
> machine-specific and known by the board code. Thus the total length can be
> derived from the FIT image and knowledge of the platform. The 'length'
> argument is redundant. Remove it.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  arch/arm/mach-imx/spl.c | 5 +++--
>  common/spl/spl_fit.c    | 4 ++--
>  include/spl.h           | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index
> aa2686bb92..11255798d3 100644
> --- a/arch/arm/mach-imx/spl.c
> +++ b/arch/arm/mach-imx/spl.c
> @@ -18,6 +18,7 @@
>  #include <asm/mach-imx/hab.h>
>  #include <asm/mach-imx/boot_mode.h>
>  #include <g_dnl.h>
> +#include <linux/libfdt.h>
> 
>  DECLARE_GLOBAL_DATA_PTR;
> 
> @@ -318,9 +319,9 @@ ulong board_spl_fit_size_align(ulong size)
>  	return size;
>  }
> 
> -void board_spl_fit_post_load(ulong load_addr, size_t length)
> +void board_spl_fit_post_load(const void *fit)
>  {
> -	u32 offset = length - CONFIG_CSF_SIZE;
> +	u32 offset = ALIGN(fdt_totalsize(fit), 0x1000);
> 
>  	if (imx_hab_authenticate_image(load_addr,
>  				       offset + IVT_SIZE + CSF_PAD_SIZE, diff --git
> a/common/spl/spl_fit.c b/common/spl/spl_fit.c index
> 795e2922ce..1b4a7f6b15 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -26,7 +26,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
>  #endif
> 
> -__weak void board_spl_fit_post_load(ulong load_addr, size_t length)
> +__weak void board_spl_fit_post_load(const void *fit)
>  {
>  }
> 
> @@ -722,7 +722,7 @@ int spl_load_simple_fit(struct spl_image_info
> *spl_image,
>  	spl_image->flags |= SPL_FIT_FOUND;
> 
>  #ifdef CONFIG_IMX_HAB
> -	board_spl_fit_post_load((ulong)fit, size);
> +	board_spl_fit_post_load(fit);
>  #endif
> 
>  	return 0;
> diff --git a/include/spl.h b/include/spl.h index 374a295fa3..f63829a99e
> 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -632,7 +632,7 @@ int board_return_to_bootrom(struct spl_image_info
> *spl_image,
>   * board_spl_fit_post_load - allow process images after loading finished
>   *
>   */
> -void board_spl_fit_post_load(ulong load_addr, size_t length);
> +void board_spl_fit_post_load(const void *fit);
> 
>  /**
>   * board_spl_fit_size_align - specific size align before processing payload
> --

Looks good to me! 

Reviewed-by: Peng Fan <peng.fan@nxp.com>

BTW: has this been tested?

Thanks,
Peng.

[PATCH 1/8] spl: fit: Drop 'length' argument to board_spl_fit_post_load()

[Alex G.]

On 12/16/20 1:13 AM, Peng Fan wrote:
>> Subject: [PATCH 1/8] spl: fit: Drop 'length' argument to
>> board_spl_fit_post_load()
>>
>> The size is derived from the FIT image itself. Any alignment requirements are
>> machine-specific and known by the board code. Thus the total length can be
>> derived from the FIT image and knowledge of the platform. The 'length'
>> argument is redundant. Remove it.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   arch/arm/mach-imx/spl.c | 5 +++--
>>   common/spl/spl_fit.c    | 4 ++--
>>   include/spl.h           | 2 +-
>>   3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index
>> aa2686bb92..11255798d3 100644
>> --- a/arch/arm/mach-imx/spl.c
>> +++ b/arch/arm/mach-imx/spl.c
>> @@ -18,6 +18,7 @@
>>   #include <asm/mach-imx/hab.h>
>>   #include <asm/mach-imx/boot_mode.h>
>>   #include <g_dnl.h>
>> +#include <linux/libfdt.h>
>>
>>   DECLARE_GLOBAL_DATA_PTR;
>>
>> @@ -318,9 +319,9 @@ ulong board_spl_fit_size_align(ulong size)
>>   	return size;
>>   }
>>
>> -void board_spl_fit_post_load(ulong load_addr, size_t length)
>> +void board_spl_fit_post_load(const void *fit)
>>   {
>> -	u32 offset = length - CONFIG_CSF_SIZE;
>> +	u32 offset = ALIGN(fdt_totalsize(fit), 0x1000);
>>
>>   	if (imx_hab_authenticate_image(load_addr,
>>   				       offset + IVT_SIZE + CSF_PAD_SIZE, diff --git
>> a/common/spl/spl_fit.c b/common/spl/spl_fit.c index
>> 795e2922ce..1b4a7f6b15 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -26,7 +26,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>   #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
>>   #endif
>>
>> -__weak void board_spl_fit_post_load(ulong load_addr, size_t length)
>> +__weak void board_spl_fit_post_load(const void *fit)
>>   {
>>   }
>>
>> @@ -722,7 +722,7 @@ int spl_load_simple_fit(struct spl_image_info
>> *spl_image,
>>   	spl_image->flags |= SPL_FIT_FOUND;
>>
>>   #ifdef CONFIG_IMX_HAB
>> -	board_spl_fit_post_load((ulong)fit, size);
>> +	board_spl_fit_post_load(fit);
>>   #endif
>>
>>   	return 0;
>> diff --git a/include/spl.h b/include/spl.h index 374a295fa3..f63829a99e
>> 100644
>> --- a/include/spl.h
>> +++ b/include/spl.h
>> @@ -632,7 +632,7 @@ int board_return_to_bootrom(struct spl_image_info
>> *spl_image,
>>    * board_spl_fit_post_load - allow process images after loading finished
>>    *
>>    */
>> -void board_spl_fit_post_load(ulong load_addr, size_t length);
>> +void board_spl_fit_post_load(const void *fit);
>>
>>   /**
>>    * board_spl_fit_size_align - specific size align before processing payload
>> --
> 
> Looks good to me!
> 
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> 
> BTW: has this been tested?

Hi Peng,

It would be great to get some independent test results, since I don't 
have any IMX hardware.

Alex

> Thanks,
> Peng.
> 

[PATCH 1/8] spl: fit: Drop 'length' argument to board_spl_fit_post_load()

[Simon Glass]

Hi Alexandru,

On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> The size is derived from the FIT image itself. Any alignment
> requirements are machine-specific and known by the board code. Thus
> the total length can be derived from the FIT image and knowledge of
> the platform. The 'length' argument is redundant. Remove it.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  arch/arm/mach-imx/spl.c | 5 +++--
>  common/spl/spl_fit.c    | 4 ++--
>  include/spl.h           | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
> index aa2686bb92..11255798d3 100644
> --- a/arch/arm/mach-imx/spl.c
> +++ b/arch/arm/mach-imx/spl.c
> @@ -18,6 +18,7 @@
>  #include <asm/mach-imx/hab.h>
>  #include <asm/mach-imx/boot_mode.h>
>  #include <g_dnl.h>
> +#include <linux/libfdt.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -318,9 +319,9 @@ ulong board_spl_fit_size_align(ulong size)
>         return size;
>  }
>
> -void board_spl_fit_post_load(ulong load_addr, size_t length)
> +void board_spl_fit_post_load(const void *fit)
>  {
> -       u32 offset = length - CONFIG_CSF_SIZE;
> +       u32 offset = ALIGN(fdt_totalsize(fit), 0x1000);
>
>         if (imx_hab_authenticate_image(load_addr,
>                                        offset + IVT_SIZE + CSF_PAD_SIZE,
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 795e2922ce..1b4a7f6b15 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -26,7 +26,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define CONFIG_SYS_BOOTM_LEN   (64 << 20)
>  #endif
>
> -__weak void board_spl_fit_post_load(ulong load_addr, size_t length)
> +__weak void board_spl_fit_post_load(const void *fit)
>  {
>  }
>
> @@ -722,7 +722,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>         spl_image->flags |= SPL_FIT_FOUND;
>
>  #ifdef CONFIG_IMX_HAB
> -       board_spl_fit_post_load((ulong)fit, size);
> +       board_spl_fit_post_load(fit);
>  #endif
>
>         return 0;
> diff --git a/include/spl.h b/include/spl.h
> index 374a295fa3..f63829a99e 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -632,7 +632,7 @@ int board_return_to_bootrom(struct spl_image_info *spl_image,
>   * board_spl_fit_post_load - allow process images after loading finished
>   *
>   */
> -void board_spl_fit_post_load(ulong load_addr, size_t length);
> +void board_spl_fit_post_load(const void *fit);

Can you please add a comment for this?


>
>  /**
>   * board_spl_fit_size_align - specific size align before processing payload
> --
> 2.26.2
>

[PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Alexandru Gagniuc]

The logical steps in spl_load_simple_fit() are difficult to follow.
I think the long comments, ifdefs, and ungodly number of variables
seriously affect the readability. In particular, it violates section 6
of the coding style, paragraphs (3), and (4).

The purpose of this patch is to improve the situation by
  - Factoring out initialization and parsing to separate functions
  - Reduce the number of variables by using a context structure
This change introduces no functional changes.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl_fit.c | 87 ++++++++++++++++++++++++++++++--------------
 1 file changed, 60 insertions(+), 27 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 1b4a7f6b15..a6f85b6f9d 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -26,6 +26,12 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
 #endif
 
+struct spl_fit_info {
+	const void *fit;
+	size_t ext_data_offset;
+	int images_node;
+};
+
 __weak void board_spl_fit_post_load(const void *fit)
 {
 }
@@ -521,28 +527,22 @@ __weak bool spl_load_simple_fit_skip_processing(void)
 	return false;
 }
 
-int spl_load_simple_fit(struct spl_image_info *spl_image,
-			struct spl_load_info *info, ulong sector, void *fit)
+static int spl_simple_fit_read(struct spl_fit_info *ctx,
+			       struct spl_load_info *info, ulong sector,
+			       const void *fit_header)
 {
+	unsigned long count, size;
 	int sectors;
-	ulong size, hsize;
-	unsigned long count;
-	struct spl_image_info image_info;
-	int node = -1;
-	int images, ret;
-	int base_offset;
-	int index = 0;
-	int firmware_node;
+	void *buf;
 
 	/*
 	 * For FIT with external data, figure out where the external images
 	 * start. This is the base for the data-offset properties in each
 	 * image.
 	 */
-	size = fdt_totalsize(fit);
-	size = (size + 3) & ~3;
+	size = ALIGN(fdt_totalsize(fit_header), 4);
 	size = board_spl_fit_size_align(size);
-	base_offset = (size + 3) & ~3;
+	ctx->ext_data_offset = ALIGN(size, 4);
 
 	/*
 	 * So far we only have one block of data from the FIT. Read the entire
@@ -552,36 +552,69 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * For FIT with external data, data is not loaded in this step.
 	 */
 	sectors = get_aligned_image_size(info, size, 0);
-	hsize = sectors * info->bl_len;
-	fit = spl_get_fit_load_buffer(hsize);
-	count = info->read(info, sector, sectors, fit);
+	buf = spl_get_fit_load_buffer(sectors * info->bl_len);
+
+	count = info->read(info, sector, sectors, buf);
+	ctx->fit = buf;
 	debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu, size=0x%lx\n",
-	      sector, sectors, fit, count, size);
+	      sector, sectors, buf, count, size);
 
 	if (count == 0)
 		return -EIO;
 
-	/* skip further processing if requested to enable load-only use cases */
-	if (spl_load_simple_fit_skip_processing())
-		return 0;
+	return 0;
+}
 
+static int spl_simple_fit_parse(struct spl_fit_info *ctx)
+{
 	if (IS_ENABLED(CONFIG_SPL_FIT_SIGNATURE)) {
-		int conf_offset = fit_find_config_node(fit);
+		int conf_offset = fit_find_config_node(ctx->fit);
 
 		printf("## Checking hash(es) for config %s ... ",
-		       fit_get_name(fit, conf_offset, NULL));
-		if (fit_config_verify(fit, conf_offset))
+		       fit_get_name(ctx->fit, conf_offset, NULL));
+		if (fit_config_verify(ctx->fit, conf_offset))
 			return -EPERM;
 		puts("OK\n");
 	}
 
 	/* find the node holding the images information */
-	images = fdt_path_offset(fit, FIT_IMAGES_PATH);
-	if (images < 0) {
-		debug("%s: Cannot find /images node: %d\n", __func__, images);
-		return -1;
+	ctx->images_node = fdt_path_offset(ctx->fit, FIT_IMAGES_PATH);
+	if (ctx->images_node < 0) {
+		debug("%s: Cannot find /images node: %d\n", __func__,
+		      ctx->images_node);
+		return -EINVAL;
 	}
 
+	return 0;
+}
+
+int spl_load_simple_fit(struct spl_image_info *spl_image,
+			struct spl_load_info *info, ulong sector, void *fit)
+{
+	struct spl_image_info image_info;
+	struct spl_fit_info ctx;
+	int node = -1;
+	int images, ret;
+	int base_offset;
+	int index = 0;
+	int firmware_node;
+
+	ret = spl_simple_fit_read(&ctx, info, sector, fit);
+	if (ret < 0)
+		return ret;
+
+	/* skip further processing if requested to enable load-only use cases */
+	if (spl_load_simple_fit_skip_processing())
+		return 0;
+
+	ret = spl_simple_fit_parse(&ctx);
+	if (ret < 0)
+		return ret;
+
+	images = ctx.images_node;
+	fit = (void *)ctx.fit;
+	base_offset = ctx.ext_data_offset;
+
 #ifdef CONFIG_SPL_FPGA
 	node = spl_fit_get_image_node(fit, images, "fpga", 0);
 	if (node >= 0) {
-- 
2.26.2

[PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Simon Glass]

Hi Alexandru,

On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> The logical steps in spl_load_simple_fit() are difficult to follow.
> I think the long comments, ifdefs, and ungodly number of variables
> seriously affect the readability. In particular, it violates section 6
> of the coding style, paragraphs (3), and (4).
>
> The purpose of this patch is to improve the situation by
>   - Factoring out initialization and parsing to separate functions
>   - Reduce the number of variables by using a context structure
> This change introduces no functional changes.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/spl/spl_fit.c | 87 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 60 insertions(+), 27 deletions(-)

This certainly looks a lot better although your email address does not
inspire confidence...

Do you think you could look at creating a sandbox SPL test for this?
It should be possible to write it in C, set up a bit of data, call
your function and check the results.

>
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 1b4a7f6b15..a6f85b6f9d 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -26,6 +26,12 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define CONFIG_SYS_BOOTM_LEN   (64 << 20)
>  #endif
>
> +struct spl_fit_info {
> +       const void *fit;
> +       size_t ext_data_offset;
> +       int images_node;
> +};

struct comments

[PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Alex G.]

On 12/18/20 8:28 PM, Simon Glass wrote:
> Hi Alexandru,
> 
> On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>
>> The logical steps in spl_load_simple_fit() are difficult to follow.
>> I think the long comments, ifdefs, and ungodly number of variables
>> seriously affect the readability. In particular, it violates section 6
>> of the coding style, paragraphs (3), and (4).
>>
>> The purpose of this patch is to improve the situation by
>>    - Factoring out initialization and parsing to separate functions
>>    - Reduce the number of variables by using a context structure
>> This change introduces no functional changes.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   common/spl/spl_fit.c | 87 ++++++++++++++++++++++++++++++--------------
>>   1 file changed, 60 insertions(+), 27 deletions(-)
> 
> This certainly looks a lot better although your email address does not
> inspire confidence...

Don't worry. It doesn't bite.

> Do you think you could look at creating a sandbox SPL test for this?
> It should be possible to write it in C, set up a bit of data, call
> your function and check the results.

I can look at it. I can't promise anything though, since this is the 
first time I heard of the sandbox. Maybe doc knows more.

Alex

>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index 1b4a7f6b15..a6f85b6f9d 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -26,6 +26,12 @@ DECLARE_GLOBAL_DATA_PTR;
>>   #define CONFIG_SYS_BOOTM_LEN   (64 << 20)
>>   #endif
>>
>> +struct spl_fit_info {
>> +       const void *fit;
>> +       size_t ext_data_offset;
>> +       int images_node;
>> +};
> 
> struct comments
> 

[PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Simon Glass]

Hi Alex,

On Mon, 21 Dec 2020 at 12:28, Alex G. <mr.nuke.me@gmail.com> wrote:
>
> On 12/18/20 8:28 PM, Simon Glass wrote:
> > Hi Alexandru,
> >
> > On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
> >>
> >> The logical steps in spl_load_simple_fit() are difficult to follow.
> >> I think the long comments, ifdefs, and ungodly number of variables
> >> seriously affect the readability. In particular, it violates section 6
> >> of the coding style, paragraphs (3), and (4).
> >>
> >> The purpose of this patch is to improve the situation by
> >>    - Factoring out initialization and parsing to separate functions
> >>    - Reduce the number of variables by using a context structure
> >> This change introduces no functional changes.
> >>
> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> >> ---
> >>   common/spl/spl_fit.c | 87 ++++++++++++++++++++++++++++++--------------
> >>   1 file changed, 60 insertions(+), 27 deletions(-)
> >
> > This certainly looks a lot better although your email address does not
> > inspire confidence...
>
> Don't worry. It doesn't bite.
>
> > Do you think you could look at creating a sandbox SPL test for this?
> > It should be possible to write it in C, set up a bit of data, call
> > your function and check the results.
>
> I can look at it. I can't promise anything though, since this is the
> first time I heard of the sandbox. Maybe doc knows more.

Yes, see doc/arch/sandbox.rst

test/dm/Makefile shows that only one test file is enabled for SPL, but
you can add more. Let me know if you need pointers.

These aliases might help, if you build into /tmp/b/<board> :

# Run a pytest on sandbox
# $1: Name of test to run (optional, else run all)

function pyt {
test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"}
}

# Run a pytest on sandbox_spl
# $1: Name of test to run  (optional, else run all SPL tests)
function pytspl {
local run=$1

[[ -z "$run" ]] && run=spl
test/py/test.py -B sandbox_spl --build-dir /tmp/b/sandbox_spl -k $run
}

Regards,
Simon

>
> Alex
>
> >>
> >> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> >> index 1b4a7f6b15..a6f85b6f9d 100644
> >> --- a/common/spl/spl_fit.c
> >> +++ b/common/spl/spl_fit.c
> >> @@ -26,6 +26,12 @@ DECLARE_GLOBAL_DATA_PTR;
> >>   #define CONFIG_SYS_BOOTM_LEN   (64 << 20)
> >>   #endif
> >>
> >> +struct spl_fit_info {
> >> +       const void *fit;
> >> +       size_t ext_data_offset;
> >> +       int images_node;
> >> +};
> >
> > struct comments
> >

Regards,
Simon

[PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Alex G.]

On 12/21/20 2:23 PM, Simon Glass wrote:
> Hi Alex,
> 
> On Mon, 21 Dec 2020 at 12:28, Alex G. <mr.nuke.me@gmail.com> wrote:
>>
>> On 12/18/20 8:28 PM, Simon Glass wrote:
>>> Hi Alexandru,
>>>
>>> On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>>>
>>>> The logical steps in spl_load_simple_fit() are difficult to follow.
>>>> I think the long comments, ifdefs, and ungodly number of variables
>>>> seriously affect the readability. In particular, it violates section 6
>>>> of the coding style, paragraphs (3), and (4).
>>>>
>>>> The purpose of this patch is to improve the situation by
>>>>     - Factoring out initialization and parsing to separate functions
>>>>     - Reduce the number of variables by using a context structure
>>>> This change introduces no functional changes.
>>>>
>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>>> ---
>>>>    common/spl/spl_fit.c | 87 ++++++++++++++++++++++++++++++--------------
>>>>    1 file changed, 60 insertions(+), 27 deletions(-)
>>>
>>> This certainly looks a lot better although your email address does not
>>> inspire confidence...
>>
>> Don't worry. It doesn't bite.
>>
>>> Do you think you could look at creating a sandbox SPL test for this?
>>> It should be possible to write it in C, set up a bit of data, call
>>> your function and check the results.
>>
>> I can look at it. I can't promise anything though, since this is the
>> first time I heard of the sandbox. Maybe doc knows more.
> 
> Yes, see doc/arch/sandbox.rst
> 
> test/dm/Makefile shows that only one test file is enabled for SPL, but
> you can add more. Let me know if you need pointers.
> 
> These aliases might help, if you build into /tmp/b/<board> :
> 
> # Run a pytest on sandbox
> # $1: Name of test to run (optional, else run all)
> 
> function pyt {
> test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"}
> }
> 
> # Run a pytest on sandbox_spl
> # $1: Name of test to run  (optional, else run all SPL tests)
> function pytspl {
> local run=$1
> 
> [[ -z "$run" ]] && run=spl
> test/py/test.py -B sandbox_spl --build-dir /tmp/b/sandbox_spl -k $run
> }

You're thinking way ahead of where I am. I know how to build a board, 
but I've never used the test infrastructure. After some fumbling, I 
figured I'd try sandbox_spl:

	$ test/py/test.py -B sandbox_spl --bd sandbox_spl --build

As you can imagine, it kept complaining about SDL. I've never used 
environment variables with Kbuild, so using NO_SPL=1 seems unnatural. 
But then why would we need SDL for testing an SPL build anyway? 'swig' 
was missing too, but that was an easy fix.

Second try:

	$ NO_SDL=1 test/py/test.py -B sandbox_spl --bd sandbox_spl \
		--build

Went a bit better, but " 29 failed, 502 passed, 212 skipped". Is this 
normal?

What I seem to be missing is how to connect this python to calling 
spl_load_simple_fit(). In the real world, I'd build u-boot and feed it a 
FIT image -- boots, okay.

Alex

> Regards,
> Simon
> 
>>
>> Alex
>>
>>>>
>>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>>> index 1b4a7f6b15..a6f85b6f9d 100644
>>>> --- a/common/spl/spl_fit.c
>>>> +++ b/common/spl/spl_fit.c
>>>> @@ -26,6 +26,12 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>    #define CONFIG_SYS_BOOTM_LEN   (64 << 20)
>>>>    #endif
>>>>
>>>> +struct spl_fit_info {
>>>> +       const void *fit;
>>>> +       size_t ext_data_offset;
>>>> +       int images_node;
>>>> +};
>>>
>>> struct comments
>>>
> 
> Regards,
> Simon
> 

[PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Simon Glass]

Hi Alex,

On Mon, 21 Dec 2020 at 15:24, Alex G. <mr.nuke.me@gmail.com> wrote:
>
>
>
> On 12/21/20 2:23 PM, Simon Glass wrote:
> > Hi Alex,
> >
> > On Mon, 21 Dec 2020 at 12:28, Alex G. <mr.nuke.me@gmail.com> wrote:
> >>
> >> On 12/18/20 8:28 PM, Simon Glass wrote:
> >>> Hi Alexandru,
> >>>
> >>> On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
> >>>>
> >>>> The logical steps in spl_load_simple_fit() are difficult to follow.
> >>>> I think the long comments, ifdefs, and ungodly number of variables
> >>>> seriously affect the readability. In particular, it violates section 6
> >>>> of the coding style, paragraphs (3), and (4).
> >>>>
> >>>> The purpose of this patch is to improve the situation by
> >>>>     - Factoring out initialization and parsing to separate functions
> >>>>     - Reduce the number of variables by using a context structure
> >>>> This change introduces no functional changes.
> >>>>
> >>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> >>>> ---
> >>>>    common/spl/spl_fit.c | 87 ++++++++++++++++++++++++++++++--------------
> >>>>    1 file changed, 60 insertions(+), 27 deletions(-)
> >>>
> >>> This certainly looks a lot better although your email address does not
> >>> inspire confidence...
> >>
> >> Don't worry. It doesn't bite.
> >>
> >>> Do you think you could look at creating a sandbox SPL test for this?
> >>> It should be possible to write it in C, set up a bit of data, call
> >>> your function and check the results.
> >>
> >> I can look at it. I can't promise anything though, since this is the
> >> first time I heard of the sandbox. Maybe doc knows more.
> >
> > Yes, see doc/arch/sandbox.rst
> >
> > test/dm/Makefile shows that only one test file is enabled for SPL, but
> > you can add more. Let me know if you need pointers.
> >
> > These aliases might help, if you build into /tmp/b/<board> :
> >
> > # Run a pytest on sandbox
> > # $1: Name of test to run (optional, else run all)
> >
> > function pyt {
> > test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"}
> > }
> >
> > # Run a pytest on sandbox_spl
> > # $1: Name of test to run  (optional, else run all SPL tests)
> > function pytspl {
> > local run=$1
> >
> > [[ -z "$run" ]] && run=spl
> > test/py/test.py -B sandbox_spl --build-dir /tmp/b/sandbox_spl -k $run
> > }
>
> You're thinking way ahead of where I am. I know how to build a board,
> but I've never used the test infrastructure. After some fumbling, I
> figured I'd try sandbox_spl:
>
>         $ test/py/test.py -B sandbox_spl --bd sandbox_spl --build
>
> As you can imagine, it kept complaining about SDL. I've never used
> environment variables with Kbuild, so using NO_SPL=1 seems unnatural.
> But then why would we need SDL for testing an SPL build anyway? 'swig'
> was missing too, but that was an easy fix.
>
> Second try:
>
>         $ NO_SDL=1 test/py/test.py -B sandbox_spl --bd sandbox_spl \
>                 --build
>
> Went a bit better, but " 29 failed, 502 passed, 212 skipped". Is this
> normal?
>
> What I seem to be missing is how to connect this python to calling
> spl_load_simple_fit(). In the real world, I'd build u-boot and feed it a
> FIT image -- boots, okay.

Here's a suggestoin
- Write a function that calls the function to load a fit and does some
checks that it worked correct, e.g. by looking in memory
- put a call to that function in an SPL C test (as mentioned ealler)

I suppose you could also boot it, perhaps by switching sandbox to use
FIT to boot?

Regards,
Simon

[PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Alex G.]

On 12/28/20 9:33 PM, Simon Glass wrote:
> Hi Alex,
> 
> On Mon, 21 Dec 2020 at 15:24, Alex G. <mr.nuke.me@gmail.com> wrote:
>>
>>
>>
>> On 12/21/20 2:23 PM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On Mon, 21 Dec 2020 at 12:28, Alex G. <mr.nuke.me@gmail.com> wrote:
>>>>
>>>> On 12/18/20 8:28 PM, Simon Glass wrote:
>>>>> Hi Alexandru,
>>>>>
>>>>> On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>>>>>
>>>>>> The logical steps in spl_load_simple_fit() are difficult to follow.
>>>>>> I think the long comments, ifdefs, and ungodly number of variables
>>>>>> seriously affect the readability. In particular, it violates section 6
>>>>>> of the coding style, paragraphs (3), and (4).
>>>>>>
>>>>>> The purpose of this patch is to improve the situation by
>>>>>>      - Factoring out initialization and parsing to separate functions
>>>>>>      - Reduce the number of variables by using a context structure
>>>>>> This change introduces no functional changes.
>>>>>>
>>>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>>>>> ---
>>>>>>     common/spl/spl_fit.c | 87 ++++++++++++++++++++++++++++++--------------
>>>>>>     1 file changed, 60 insertions(+), 27 deletions(-)
>>>>>
>>>>> This certainly looks a lot better although your email address does not
>>>>> inspire confidence...
>>>>
>>>> Don't worry. It doesn't bite.
>>>>
>>>>> Do you think you could look at creating a sandbox SPL test for this?
>>>>> It should be possible to write it in C, set up a bit of data, call
>>>>> your function and check the results.
>>>>
>>>> I can look at it. I can't promise anything though, since this is the
>>>> first time I heard of the sandbox. Maybe doc knows more.
>>>
>>> Yes, see doc/arch/sandbox.rst
>>>
>>> test/dm/Makefile shows that only one test file is enabled for SPL, but
>>> you can add more. Let me know if you need pointers.
>>>
>>> These aliases might help, if you build into /tmp/b/<board> :
>>>
>>> # Run a pytest on sandbox
>>> # $1: Name of test to run (optional, else run all)
>>>
>>> function pyt {
>>> test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"}
>>> }
>>>
>>> # Run a pytest on sandbox_spl
>>> # $1: Name of test to run  (optional, else run all SPL tests)
>>> function pytspl {
>>> local run=$1
>>>
>>> [[ -z "$run" ]] && run=spl
>>> test/py/test.py -B sandbox_spl --build-dir /tmp/b/sandbox_spl -k $run
>>> }
>>
>> You're thinking way ahead of where I am. I know how to build a board,
>> but I've never used the test infrastructure. After some fumbling, I
>> figured I'd try sandbox_spl:
>>
>>          $ test/py/test.py -B sandbox_spl --bd sandbox_spl --build
>>
>> As you can imagine, it kept complaining about SDL. I've never used
>> environment variables with Kbuild, so using NO_SPL=1 seems unnatural.
>> But then why would we need SDL for testing an SPL build anyway? 'swig'
>> was missing too, but that was an easy fix.
>>
>> Second try:
>>
>>          $ NO_SDL=1 test/py/test.py -B sandbox_spl --bd sandbox_spl \
>>                  --build
>>
>> Went a bit better, but " 29 failed, 502 passed, 212 skipped". Is this
>> normal?
>>
>> What I seem to be missing is how to connect this python to calling
>> spl_load_simple_fit(). In the real world, I'd build u-boot and feed it a
>> FIT image -- boots, okay.
> 
> Here's a suggestoin
> - Write a function that calls the function to load a fit and does some
> checks that it worked correct, e.g. by looking in memory
> - put a call to that function in an SPL C test (as mentioned ealler)
> 
> I suppose you could also boot it, perhaps by switching sandbox to use
> FIT to boot?

Hi Simon,

There seems to be a lot more to wrapping around spl_load_simple_fit(). 
We need populated spl_image_info spl_load_info structure. I'm not even 
sure if the test code runs in SPL, or how to run it in SPL.

There are examples, and unfocused documentation on how to connect this 
into u-boot proper. The current documentation and exampples are not 
helping with what I was trying to accomplish. Unfortunately, I've spent 
a week on this, and wasn't able to make any progress. I'm one guy who's 
getting paid to ship a product. This test infrastructure is more tedious 
than I anticipated, and I need to move on.

ALex

[PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Tom Rini]

On Tue, Dec 29, 2020 at 06:07:08PM -0600, Alex G. wrote:
> 
> 
> On 12/28/20 9:33 PM, Simon Glass wrote:
> > Hi Alex,
> > 
> > On Mon, 21 Dec 2020 at 15:24, Alex G. <mr.nuke.me@gmail.com> wrote:
> > > 
> > > 
> > > 
> > > On 12/21/20 2:23 PM, Simon Glass wrote:
> > > > Hi Alex,
> > > > 
> > > > On Mon, 21 Dec 2020 at 12:28, Alex G. <mr.nuke.me@gmail.com> wrote:
> > > > > 
> > > > > On 12/18/20 8:28 PM, Simon Glass wrote:
> > > > > > Hi Alexandru,
> > > > > > 
> > > > > > On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
> > > > > > > 
> > > > > > > The logical steps in spl_load_simple_fit() are difficult to follow.
> > > > > > > I think the long comments, ifdefs, and ungodly number of variables
> > > > > > > seriously affect the readability. In particular, it violates section 6
> > > > > > > of the coding style, paragraphs (3), and (4).
> > > > > > > 
> > > > > > > The purpose of this patch is to improve the situation by
> > > > > > >      - Factoring out initialization and parsing to separate functions
> > > > > > >      - Reduce the number of variables by using a context structure
> > > > > > > This change introduces no functional changes.
> > > > > > > 
> > > > > > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> > > > > > > ---
> > > > > > >     common/spl/spl_fit.c | 87 ++++++++++++++++++++++++++++++--------------
> > > > > > >     1 file changed, 60 insertions(+), 27 deletions(-)
> > > > > > 
> > > > > > This certainly looks a lot better although your email address does not
> > > > > > inspire confidence...
> > > > > 
> > > > > Don't worry. It doesn't bite.
> > > > > 
> > > > > > Do you think you could look at creating a sandbox SPL test for this?
> > > > > > It should be possible to write it in C, set up a bit of data, call
> > > > > > your function and check the results.
> > > > > 
> > > > > I can look at it. I can't promise anything though, since this is the
> > > > > first time I heard of the sandbox. Maybe doc knows more.
> > > > 
> > > > Yes, see doc/arch/sandbox.rst
> > > > 
> > > > test/dm/Makefile shows that only one test file is enabled for SPL, but
> > > > you can add more. Let me know if you need pointers.
> > > > 
> > > > These aliases might help, if you build into /tmp/b/<board> :
> > > > 
> > > > # Run a pytest on sandbox
> > > > # $1: Name of test to run (optional, else run all)
> > > > 
> > > > function pyt {
> > > > test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"}
> > > > }
> > > > 
> > > > # Run a pytest on sandbox_spl
> > > > # $1: Name of test to run  (optional, else run all SPL tests)
> > > > function pytspl {
> > > > local run=$1
> > > > 
> > > > [[ -z "$run" ]] && run=spl
> > > > test/py/test.py -B sandbox_spl --build-dir /tmp/b/sandbox_spl -k $run
> > > > }
> > > 
> > > You're thinking way ahead of where I am. I know how to build a board,
> > > but I've never used the test infrastructure. After some fumbling, I
> > > figured I'd try sandbox_spl:
> > > 
> > >          $ test/py/test.py -B sandbox_spl --bd sandbox_spl --build
> > > 
> > > As you can imagine, it kept complaining about SDL. I've never used
> > > environment variables with Kbuild, so using NO_SPL=1 seems unnatural.
> > > But then why would we need SDL for testing an SPL build anyway? 'swig'
> > > was missing too, but that was an easy fix.
> > > 
> > > Second try:
> > > 
> > >          $ NO_SDL=1 test/py/test.py -B sandbox_spl --bd sandbox_spl \
> > >                  --build
> > > 
> > > Went a bit better, but " 29 failed, 502 passed, 212 skipped". Is this
> > > normal?
> > > 
> > > What I seem to be missing is how to connect this python to calling
> > > spl_load_simple_fit(). In the real world, I'd build u-boot and feed it a
> > > FIT image -- boots, okay.
> > 
> > Here's a suggestoin
> > - Write a function that calls the function to load a fit and does some
> > checks that it worked correct, e.g. by looking in memory
> > - put a call to that function in an SPL C test (as mentioned ealler)
> > 
> > I suppose you could also boot it, perhaps by switching sandbox to use
> > FIT to boot?
> 
> Hi Simon,
> 
> There seems to be a lot more to wrapping around spl_load_simple_fit(). We
> need populated spl_image_info spl_load_info structure. I'm not even sure if
> the test code runs in SPL, or how to run it in SPL.
> 
> There are examples, and unfocused documentation on how to connect this into
> u-boot proper. The current documentation and exampples are not helping with
> what I was trying to accomplish. Unfortunately, I've spent a week on this,
> and wasn't able to make any progress. I'm one guy who's getting paid to ship
> a product. This test infrastructure is more tedious than I anticipated, and
> I need to move on.

Thanks for the feedback.  We need to make the tests easier to add then.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201230/3b6d1f1b/attachment.sig>

[PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Simon Glass]

Hi Alex,

On Tue, 29 Dec 2020 at 17:07, Alex G. <mr.nuke.me@gmail.com> wrote:
>
>
>
> On 12/28/20 9:33 PM, Simon Glass wrote:
> > Hi Alex,
> >
> > On Mon, 21 Dec 2020 at 15:24, Alex G. <mr.nuke.me@gmail.com> wrote:
> >>
> >>
> >>
> >> On 12/21/20 2:23 PM, Simon Glass wrote:
> >>> Hi Alex,
> >>>
> >>> On Mon, 21 Dec 2020 at 12:28, Alex G. <mr.nuke.me@gmail.com> wrote:
> >>>>
> >>>> On 12/18/20 8:28 PM, Simon Glass wrote:
> >>>>> Hi Alexandru,
> >>>>>
> >>>>> On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
> >>>>>>
> >>>>>> The logical steps in spl_load_simple_fit() are difficult to follow.
> >>>>>> I think the long comments, ifdefs, and ungodly number of variables
> >>>>>> seriously affect the readability. In particular, it violates section 6
> >>>>>> of the coding style, paragraphs (3), and (4).
> >>>>>>
> >>>>>> The purpose of this patch is to improve the situation by
> >>>>>>      - Factoring out initialization and parsing to separate functions
> >>>>>>      - Reduce the number of variables by using a context structure
> >>>>>> This change introduces no functional changes.
> >>>>>>
> >>>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> >>>>>> ---
> >>>>>>     common/spl/spl_fit.c | 87 ++++++++++++++++++++++++++++++--------------
> >>>>>>     1 file changed, 60 insertions(+), 27 deletions(-)
> >>>>>
> >>>>> This certainly looks a lot better although your email address does not
> >>>>> inspire confidence...
> >>>>
> >>>> Don't worry. It doesn't bite.
> >>>>
> >>>>> Do you think you could look at creating a sandbox SPL test for this?
> >>>>> It should be possible to write it in C, set up a bit of data, call
> >>>>> your function and check the results.
> >>>>
> >>>> I can look at it. I can't promise anything though, since this is the
> >>>> first time I heard of the sandbox. Maybe doc knows more.
> >>>
> >>> Yes, see doc/arch/sandbox.rst
> >>>
> >>> test/dm/Makefile shows that only one test file is enabled for SPL, but
> >>> you can add more. Let me know if you need pointers.
> >>>
> >>> These aliases might help, if you build into /tmp/b/<board> :
> >>>
> >>> # Run a pytest on sandbox
> >>> # $1: Name of test to run (optional, else run all)
> >>>
> >>> function pyt {
> >>> test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"}
> >>> }
> >>>
> >>> # Run a pytest on sandbox_spl
> >>> # $1: Name of test to run  (optional, else run all SPL tests)
> >>> function pytspl {
> >>> local run=$1
> >>>
> >>> [[ -z "$run" ]] && run=spl
> >>> test/py/test.py -B sandbox_spl --build-dir /tmp/b/sandbox_spl -k $run
> >>> }
> >>
> >> You're thinking way ahead of where I am. I know how to build a board,
> >> but I've never used the test infrastructure. After some fumbling, I
> >> figured I'd try sandbox_spl:
> >>
> >>          $ test/py/test.py -B sandbox_spl --bd sandbox_spl --build
> >>
> >> As you can imagine, it kept complaining about SDL. I've never used
> >> environment variables with Kbuild, so using NO_SPL=1 seems unnatural.
> >> But then why would we need SDL for testing an SPL build anyway? 'swig'
> >> was missing too, but that was an easy fix.
> >>
> >> Second try:
> >>
> >>          $ NO_SDL=1 test/py/test.py -B sandbox_spl --bd sandbox_spl \
> >>                  --build
> >>
> >> Went a bit better, but " 29 failed, 502 passed, 212 skipped". Is this
> >> normal?
> >>
> >> What I seem to be missing is how to connect this python to calling
> >> spl_load_simple_fit(). In the real world, I'd build u-boot and feed it a
> >> FIT image -- boots, okay.
> >
> > Here's a suggestoin
> > - Write a function that calls the function to load a fit and does some
> > checks that it worked correct, e.g. by looking in memory
> > - put a call to that function in an SPL C test (as mentioned ealler)
> >
> > I suppose you could also boot it, perhaps by switching sandbox to use
> > FIT to boot?
>
> Hi Simon,
>
> There seems to be a lot more to wrapping around spl_load_simple_fit().
> We need populated spl_image_info spl_load_info structure. I'm not even
> sure if the test code runs in SPL, or how to run it in SPL.

'make qcheck' will run it

But generally I run the tests directly.

>
> There are examples, and unfocused documentation on how to connect this
> into u-boot proper. The current documentation and exampples are not
> helping with what I was trying to accomplish. Unfortunately, I've spent
> a week on this, and wasn't able to make any progress. I'm one guy who's
> getting paid to ship a product. This test infrastructure is more tedious
> than I anticipated, and I need to move on.

Sorry to hear that. The SPL tests are very new. I'll take a look at
adding some documentation for that in particular. If you had any
specific things you, or WIP code, please send it through.

One suggestion if this happens again, is to just spend an hour on it
and send out some questions, then start again when you get a response.
It isn't always possible but it would hopefully save you time.

Regards,
Simon

[PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Simon Glass]

HI Alex,

On Wed, 30 Dec 2020 at 19:57, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Alex,
>
> On Tue, 29 Dec 2020 at 17:07, Alex G. <mr.nuke.me@gmail.com> wrote:
> >
> >
> >
> > On 12/28/20 9:33 PM, Simon Glass wrote:
> > > Hi Alex,
> > >
> > > On Mon, 21 Dec 2020 at 15:24, Alex G. <mr.nuke.me@gmail.com> wrote:
> > >>
> > >>
> > >>
> > >> On 12/21/20 2:23 PM, Simon Glass wrote:
> > >>> Hi Alex,
> > >>>
> > >>> On Mon, 21 Dec 2020 at 12:28, Alex G. <mr.nuke.me@gmail.com> wrote:
> > >>>>
> > >>>> On 12/18/20 8:28 PM, Simon Glass wrote:
> > >>>>> Hi Alexandru,
> > >>>>>
> > >>>>> On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
> > >>>>>>
> > >>>>>> The logical steps in spl_load_simple_fit() are difficult to follow.
> > >>>>>> I think the long comments, ifdefs, and ungodly number of variables
> > >>>>>> seriously affect the readability. In particular, it violates section 6
> > >>>>>> of the coding style, paragraphs (3), and (4).
> > >>>>>>
> > >>>>>> The purpose of this patch is to improve the situation by
> > >>>>>>      - Factoring out initialization and parsing to separate functions
> > >>>>>>      - Reduce the number of variables by using a context structure
> > >>>>>> This change introduces no functional changes.
> > >>>>>>
> > >>>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> > >>>>>> ---
> > >>>>>>     common/spl/spl_fit.c | 87 ++++++++++++++++++++++++++++++--------------
> > >>>>>>     1 file changed, 60 insertions(+), 27 deletions(-)
> > >>>>>
> > >>>>> This certainly looks a lot better although your email address does not
> > >>>>> inspire confidence...
> > >>>>
> > >>>> Don't worry. It doesn't bite.
> > >>>>
> > >>>>> Do you think you could look at creating a sandbox SPL test for this?
> > >>>>> It should be possible to write it in C, set up a bit of data, call
> > >>>>> your function and check the results.
> > >>>>
> > >>>> I can look at it. I can't promise anything though, since this is the
> > >>>> first time I heard of the sandbox. Maybe doc knows more.
> > >>>
> > >>> Yes, see doc/arch/sandbox.rst
> > >>>
> > >>> test/dm/Makefile shows that only one test file is enabled for SPL, but
> > >>> you can add more. Let me know if you need pointers.
> > >>>
> > >>> These aliases might help, if you build into /tmp/b/<board> :
> > >>>
> > >>> # Run a pytest on sandbox
> > >>> # $1: Name of test to run (optional, else run all)
> > >>>
> > >>> function pyt {
> > >>> test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"}
> > >>> }
> > >>>
> > >>> # Run a pytest on sandbox_spl
> > >>> # $1: Name of test to run  (optional, else run all SPL tests)
> > >>> function pytspl {
> > >>> local run=$1
> > >>>
> > >>> [[ -z "$run" ]] && run=spl
> > >>> test/py/test.py -B sandbox_spl --build-dir /tmp/b/sandbox_spl -k $run
> > >>> }
> > >>
> > >> You're thinking way ahead of where I am. I know how to build a board,
> > >> but I've never used the test infrastructure. After some fumbling, I
> > >> figured I'd try sandbox_spl:
> > >>
> > >>          $ test/py/test.py -B sandbox_spl --bd sandbox_spl --build
> > >>
> > >> As you can imagine, it kept complaining about SDL. I've never used
> > >> environment variables with Kbuild, so using NO_SPL=1 seems unnatural.
> > >> But then why would we need SDL for testing an SPL build anyway? 'swig'
> > >> was missing too, but that was an easy fix.
> > >>
> > >> Second try:
> > >>
> > >>          $ NO_SDL=1 test/py/test.py -B sandbox_spl --bd sandbox_spl \
> > >>                  --build
> > >>
> > >> Went a bit better, but " 29 failed, 502 passed, 212 skipped". Is this
> > >> normal?
> > >>
> > >> What I seem to be missing is how to connect this python to calling
> > >> spl_load_simple_fit(). In the real world, I'd build u-boot and feed it a
> > >> FIT image -- boots, okay.
> > >
> > > Here's a suggestoin
> > > - Write a function that calls the function to load a fit and does some
> > > checks that it worked correct, e.g. by looking in memory
> > > - put a call to that function in an SPL C test (as mentioned ealler)
> > >
> > > I suppose you could also boot it, perhaps by switching sandbox to use
> > > FIT to boot?
> >
> > Hi Simon,
> >
> > There seems to be a lot more to wrapping around spl_load_simple_fit().
> > We need populated spl_image_info spl_load_info structure. I'm not even
> > sure if the test code runs in SPL, or how to run it in SPL.
>
> 'make qcheck' will run it
>
> But generally I run the tests directly.
>
> >
> > There are examples, and unfocused documentation on how to connect this
> > into u-boot proper. The current documentation and exampples are not
> > helping with what I was trying to accomplish. Unfortunately, I've spent
> > a week on this, and wasn't able to make any progress. I'm one guy who's
> > getting paid to ship a product. This test infrastructure is more tedious
> > than I anticipated, and I need to move on.
>
> Sorry to hear that. The SPL tests are very new. I'll take a look at
> adding some documentation for that in particular. If you had any
> specific things you, or WIP code, please send it through.
>
> One suggestion if this happens again, is to just spend an hour on it
> and send out some questions, then start again when you get a response.
> It isn't always possible but it would hopefully save you time.

Just to follow up on this email, I have sent a v2 series with a simple
FIT test and some documentation updates. I hope it i useful.

Regards,
Simon

[PATCH 3/8] spl: fit: Pass FIT context via a structure pointer

[Alexandru Gagniuc]

Several loose arguments describe the FIT image. They are thus related,
and it makes sense to pass them together, in a structure. Examples
include the FIT blob pointer, offset to FDT nodes, and the offset to
external data.

Use a spl_fit_info structure to group these parameters.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl_fit.c | 101 ++++++++++++++++++-------------------------
 1 file changed, 43 insertions(+), 58 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index a6f85b6f9d..e6b27f61af 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -76,7 +76,7 @@ static int find_node_from_desc(const void *fit, int node, const char *str)
  *
  * Return:	0 on success, or a negative error number
  */
-static int spl_fit_get_image_name(const void *fit, int images,
+static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
 				  const char *type, int index,
 				  const char **outname)
 {
@@ -87,21 +87,21 @@ static int spl_fit_get_image_name(const void *fit, int images,
 	int len, i;
 	bool found = true;
 
-	conf_node = fit_find_config_node(fit);
+	conf_node = fit_find_config_node(ctx->fit);
 	if (conf_node < 0) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
 		printf("No matching DT out of these options:\n");
-		for (node = fdt_first_subnode(fit, conf_node);
+		for (node = fdt_first_subnode(ctx->fit, conf_node);
 		     node >= 0;
-		     node = fdt_next_subnode(fit, node)) {
-			name = fdt_getprop(fit, node, "description", &len);
+		     node = fdt_next_subnode(ctx->fit, node)) {
+			name = fdt_getprop(ctx->fit, node, "description", &len);
 			printf("   %s\n", name);
 		}
 #endif
 		return conf_node;
 	}
 
-	name = fdt_getprop(fit, conf_node, type, &len);
+	name = fdt_getprop(ctx->fit, conf_node, type, &len);
 	if (!name) {
 		debug("cannot find property '%s': %d\n", type, len);
 		return -EINVAL;
@@ -135,11 +135,11 @@ static int spl_fit_get_image_name(const void *fit, int images,
 			 * node name.
 			 */
 			int node;
-			int images = fdt_path_offset(fit, FIT_IMAGES_PATH);
+			int images = fdt_path_offset(ctx->fit, FIT_IMAGES_PATH);
 
-			node = find_node_from_desc(fit, images, str);
+			node = find_node_from_desc(ctx->fit, images, str);
 			if (node > 0)
-				str = fdt_get_name(fit, node, NULL);
+				str = fdt_get_name(ctx->fit, node, NULL);
 
 			found = true;
 		}
@@ -166,20 +166,20 @@ static int spl_fit_get_image_name(const void *fit, int images,
  * Return:	the node offset of the respective image node or a negative
  *		error number.
  */
-static int spl_fit_get_image_node(const void *fit, int images,
+static int spl_fit_get_image_node(const struct spl_fit_info *ctx,
 				  const char *type, int index)
 {
 	const char *str;
 	int err;
 	int node;
 
-	err = spl_fit_get_image_name(fit, images, type, index, &str);
+	err = spl_fit_get_image_name(ctx, type, index, &str);
 	if (err)
 		return err;
 
 	debug("%s: '%s'\n", type, str);
 
-	node = fdt_subnode_offset(fit, images, str);
+	node = fdt_subnode_offset(ctx->fit, ctx->images_node, str);
 	if (node < 0) {
 		pr_err("cannot find image node '%s': %d\n", str, node);
 		return -EINVAL;
@@ -230,10 +230,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
  * spl_load_fit_image(): load the image described in a certain FIT node
  * @info:	points to information about the device to load data from
  * @sector:	the start sector of the FIT image on the device
- * @fit:	points to the flattened device tree blob describing the FIT
- *		image
- * @base_offset: the beginning of the data area containing the actual
- *		image data, relative to the beginning of the FIT
+ * @ctx:	points to the FIT context structure
  * @node:	offset of the DT node describing the image to load (relative
  *		to @fit)
  * @image_info:	will be filled with information about the loaded image
@@ -244,7 +241,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
  * Return:	0 on success or a negative error number.
  */
 static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
-			      void *fit, ulong base_offset, int node,
+			      const struct spl_fit_info *ctx, int node,
 			      struct spl_image_info *image_info)
 {
 	int offset;
@@ -258,6 +255,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 	int align_len = ARCH_DMA_MINALIGN - 1;
 	uint8_t image_comp = -1, type = -1;
 	const void *data;
+	const void *fit = ctx->fit;
 	bool external_data = false;
 
 	if (IS_ENABLED(CONFIG_SPL_FPGA) ||
@@ -279,7 +277,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 	if (!fit_image_get_data_position(fit, node, &offset)) {
 		external_data = true;
 	} else if (!fit_image_get_data_offset(fit, node, &offset)) {
-		offset += base_offset;
+		offset += ctx->ext_data_offset;
 		external_data = true;
 	}
 
@@ -355,7 +353,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 
 static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 			      struct spl_load_info *info, ulong sector,
-			      void *fit, int images, ulong base_offset)
+			      const struct spl_fit_info *ctx)
 {
 	struct spl_image_info image_info;
 	int node, ret = 0, index = 0;
@@ -367,7 +365,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 	image_info.load_addr = spl_image->load_addr + spl_image->size;
 
 	/* Figure out which device tree the board wants to use */
-	node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, index++);
+	node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index++);
 	if (node < 0) {
 		debug("%s: cannot find FDT node\n", __func__);
 
@@ -381,7 +379,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 		else
 			return node;
 	} else {
-		ret = spl_load_fit_image(info, sector, fit, base_offset, node,
+		ret = spl_load_fit_image(info, sector, ctx, node,
 					 &image_info);
 		if (ret < 0)
 			return ret;
@@ -394,8 +392,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 		void *tmpbuffer = NULL;
 
 		for (; ; index++) {
-			node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP,
-						      index);
+			node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index);
 			if (node == -E2BIG) {
 				debug("%s: No additional FDT node\n", __func__);
 				break;
@@ -418,7 +415,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 					      __func__);
 			}
 			image_info.load_addr = (ulong)tmpbuffer;
-			ret = spl_load_fit_image(info, sector, fit, base_offset,
+			ret = spl_load_fit_image(info, sector, ctx,
 						 node, &image_info);
 			if (ret < 0)
 				break;
@@ -433,12 +430,12 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 							(void *)image_info.load_addr);
 			if (ret) {
 				pr_err("failed to apply DT overlay %s\n",
-				       fit_get_name(fit, node, NULL));
+				       fit_get_name(ctx->fit, node, NULL));
 				break;
 			}
 
 			debug("%s: DT overlay %s applied\n", __func__,
-			      fit_get_name(fit, node, NULL));
+			      fit_get_name(ctx->fit, node, NULL));
 		}
 		free(tmpbuffer);
 		if (ret)
@@ -453,7 +450,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 	return ret;
 }
 
-static int spl_fit_record_loadable(const void *fit, int images, int index,
+static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index,
 				   void *blob, struct spl_image_info *image)
 {
 	int ret = 0;
@@ -461,17 +458,16 @@ static int spl_fit_record_loadable(const void *fit, int images, int index,
 	const char *name;
 	int node;
 
-	ret = spl_fit_get_image_name(fit, images, "loadables",
-				     index, &name);
+	ret = spl_fit_get_image_name(ctx, "loadables", index, &name);
 	if (ret < 0)
 		return ret;
 
-	node = spl_fit_get_image_node(fit, images, "loadables", index);
+	node = spl_fit_get_image_node(ctx, "loadables", index);
 
 	ret = fdt_record_loadable(blob, index, name, image->load_addr,
 				  image->size, image->entry_point,
-				  fdt_getprop(fit, node, "type", NULL),
-				  fdt_getprop(fit, node, "os", NULL));
+				  fdt_getprop(ctx->fit, node, "type", NULL),
+				  fdt_getprop(ctx->fit, node, "os", NULL));
 #endif
 	return ret;
 }
@@ -594,8 +590,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	struct spl_image_info image_info;
 	struct spl_fit_info ctx;
 	int node = -1;
-	int images, ret;
-	int base_offset;
+	int ret;
 	int index = 0;
 	int firmware_node;
 
@@ -611,16 +606,11 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	if (ret < 0)
 		return ret;
 
-	images = ctx.images_node;
-	fit = (void *)ctx.fit;
-	base_offset = ctx.ext_data_offset;
-
 #ifdef CONFIG_SPL_FPGA
-	node = spl_fit_get_image_node(fit, images, "fpga", 0);
+	node = spl_fit_get_image_node(&ctx, "fpga", 0);
 	if (node >= 0) {
 		/* Load the image and set up the spl_image structure */
-		ret = spl_load_fit_image(info, sector, fit, base_offset, node,
-					 spl_image);
+		ret = spl_load_fit_image(info, sector, ctx, node, spl_image);
 		if (ret) {
 			printf("%s: Cannot load the FPGA: %i\n", __func__, ret);
 			return ret;
@@ -649,15 +639,14 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 *   - fall back to using the first 'loadables' entry
 	 */
 	if (node < 0)
-		node = spl_fit_get_image_node(fit, images, FIT_FIRMWARE_PROP,
-					      0);
+		node = spl_fit_get_image_node(&ctx, FIT_FIRMWARE_PROP, 0);
 #ifdef CONFIG_SPL_OS_BOOT
 	if (node < 0)
-		node = spl_fit_get_image_node(fit, images, FIT_KERNEL_PROP, 0);
+		node = spl_fit_get_image_node(&ctx, FIT_KERNEL_PROP, 0);
 #endif
 	if (node < 0) {
 		debug("could not find firmware image, trying loadables...\n");
-		node = spl_fit_get_image_node(fit, images, "loadables", 0);
+		node = spl_fit_get_image_node(&ctx, "loadables", 0);
 		/*
 		 * If we pick the U-Boot image from "loadables", start at
 		 * the second image when later loading additional images.
@@ -671,8 +660,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	}
 
 	/* Load the image and set up the spl_image structure */
-	ret = spl_load_fit_image(info, sector, fit, base_offset, node,
-				 spl_image);
+	ret = spl_load_fit_image(info, sector, &ctx, node, spl_image);
 	if (ret)
 		return ret;
 
@@ -680,7 +668,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * For backward compatibility, we treat the first node that is
 	 * as a U-Boot image, if no OS-type has been declared.
 	 */
-	if (!spl_fit_image_get_os(fit, node, &spl_image->os))
+	if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os))
 		debug("Image OS is %s\n", genimg_get_os_name(spl_image->os));
 #if !defined(CONFIG_SPL_OS_BOOT)
 	else
@@ -692,8 +680,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * We allow this to fail, as the U-Boot image might embed its FDT.
 	 */
 	if (spl_image->os == IH_OS_U_BOOT) {
-		ret = spl_fit_append_fdt(spl_image, info, sector, fit,
-					 images, base_offset);
+		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
 		if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0)
 			return ret;
 	}
@@ -703,7 +690,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	for (; ; index++) {
 		uint8_t os_type = IH_OS_INVALID;
 
-		node = spl_fit_get_image_node(fit, images, "loadables", index);
+		node = spl_fit_get_image_node(&ctx, "loadables", index);
 		if (node < 0)
 			break;
 
@@ -715,17 +702,15 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 		if (firmware_node == node)
 			continue;
 
-		ret = spl_load_fit_image(info, sector, fit, base_offset, node,
-					 &image_info);
+		ret = spl_load_fit_image(info, sector, &ctx, node, &image_info);
 		if (ret < 0)
 			continue;
 
-		if (!spl_fit_image_get_os(fit, node, &os_type))
+		if (!spl_fit_image_get_os(ctx.fit, node, &os_type))
 			debug("Loadable is %s\n", genimg_get_os_name(os_type));
 
 		if (os_type == IH_OS_U_BOOT) {
-			spl_fit_append_fdt(&image_info, info, sector,
-					   fit, images, base_offset);
+			spl_fit_append_fdt(&image_info, info, sector, &ctx);
 			spl_image->fdt_addr = image_info.fdt_addr;
 		}
 
@@ -739,7 +724,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 
 		/* Record our loadables into the FDT */
 		if (spl_image->fdt_addr)
-			spl_fit_record_loadable(fit, images, index,
+			spl_fit_record_loadable(&ctx, index,
 						spl_image->fdt_addr,
 						&image_info);
 	}
@@ -755,7 +740,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	spl_image->flags |= SPL_FIT_FOUND;
 
 #ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load(fit);
+	board_spl_fit_post_load(ctx.fit);
 #endif
 
 	return 0;
-- 
2.26.2

[PATCH 3/8] spl: fit: Pass FIT context via a structure pointer

[Simon Glass]

On Tue, 15 Dec 2020 at 17:09, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> Several loose arguments describe the FIT image. They are thus related,
> and it makes sense to pass them together, in a structure. Examples
> include the FIT blob pointer, offset to FDT nodes, and the offset to
> external data.
>
> Use a spl_fit_info structure to group these parameters.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/spl/spl_fit.c | 101 ++++++++++++++++++-------------------------
>  1 file changed, 43 insertions(+), 58 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 4/8] spl: fit: Remove useless loop in spl_fit_get_image_name()

[Alexandru Gagniuc]

When a desired configuration is not found, conf_node will have a
negative value. Thus the for loop will start at the root "/" node of
the image, print the "/description" property, and stop.

It appears the intent of the loop was to print the names of the
subnodes under "/configurations". We would need the offset to the
"/configurations" node, which is abstracted by fit_find_config_node().

This change agrees that abstracting the node offset is the correct
design, and we shouldn't be parsing the configurations manually. Thus
the loop in spl_fit_get_image_name() is useless. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl_fit.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index e6b27f61af..4e27eb0b3d 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -88,18 +88,8 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
 	bool found = true;
 
 	conf_node = fit_find_config_node(ctx->fit);
-	if (conf_node < 0) {
-#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-		printf("No matching DT out of these options:\n");
-		for (node = fdt_first_subnode(ctx->fit, conf_node);
-		     node >= 0;
-		     node = fdt_next_subnode(ctx->fit, node)) {
-			name = fdt_getprop(ctx->fit, node, "description", &len);
-			printf("   %s\n", name);
-		}
-#endif
+	if (conf_node < 0)
 		return conf_node;
-	}
 
 	name = fdt_getprop(ctx->fit, conf_node, type, &len);
 	if (!name) {
-- 
2.26.2

[PATCH 4/8] spl: fit: Remove useless loop in spl_fit_get_image_name()

[Simon Glass]

On Tue, 15 Dec 2020 at 17:10, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> When a desired configuration is not found, conf_node will have a
> negative value. Thus the for loop will start at the root "/" node of
> the image, print the "/description" property, and stop.
>
> It appears the intent of the loop was to print the names of the
> subnodes under "/configurations". We would need the offset to the
> "/configurations" node, which is abstracted by fit_find_config_node().
>
> This change agrees that abstracting the node offset is the correct
> design, and we shouldn't be parsing the configurations manually. Thus
> the loop in spl_fit_get_image_name() is useless. Remove it.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/spl/spl_fit.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 5/8] spl: fit: Only look up FIT configuration node once

[Alexandru Gagniuc]

The configuration node a sub node under "/configurations", which
describes the components to load from "/images". We only need to
locate this node once.

However, for each component, spl_fit_get_image_name() would parse the
FIT image, looking for the correct node. Such work duplication is not
necessary. Instead, once the node is found, cache it, and re-use it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl_fit.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 4e27eb0b3d..4b49f369ac 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -30,6 +30,7 @@ struct spl_fit_info {
 	const void *fit;
 	size_t ext_data_offset;
 	int images_node;
+	int conf_node;
 };
 
 __weak void board_spl_fit_post_load(const void *fit)
@@ -83,15 +84,10 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
 	struct udevice *sysinfo;
 	const char *name, *str;
 	__maybe_unused int node;
-	int conf_node;
 	int len, i;
 	bool found = true;
 
-	conf_node = fit_find_config_node(ctx->fit);
-	if (conf_node < 0)
-		return conf_node;
-
-	name = fdt_getprop(ctx->fit, conf_node, type, &len);
+	name = fdt_getprop(ctx->fit, ctx->conf_node, type, &len);
 	if (!name) {
 		debug("cannot find property '%s': %d\n", type, len);
 		return -EINVAL;
@@ -553,12 +549,15 @@ static int spl_simple_fit_read(struct spl_fit_info *ctx,
 
 static int spl_simple_fit_parse(struct spl_fit_info *ctx)
 {
-	if (IS_ENABLED(CONFIG_SPL_FIT_SIGNATURE)) {
-		int conf_offset = fit_find_config_node(ctx->fit);
+	/* Find the correct subnode under "/configurations" */
+	ctx->conf_node = fit_find_config_node(ctx->fit);
+	if (ctx->conf_node < 0)
+		return -EINVAL;
 
+	if (IS_ENABLED(CONFIG_SPL_FIT_SIGNATURE)) {
 		printf("## Checking hash(es) for config %s ... ",
-		       fit_get_name(ctx->fit, conf_offset, NULL));
-		if (fit_config_verify(ctx->fit, conf_offset))
+		       fit_get_name(ctx->fit, ctx->conf_node, NULL));
+		if (fit_config_verify(ctx->fit, ctx->conf_node))
 			return -EPERM;
 		puts("OK\n");
 	}
-- 
2.26.2

[PATCH 5/8] spl: fit: Only look up FIT configuration node once

[Simon Glass]

On Tue, 15 Dec 2020 at 17:10, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> The configuration node a sub node under "/configurations", which
> describes the components to load from "/images". We only need to
> locate this node once.
>
> However, for each component, spl_fit_get_image_name() would parse the
> FIT image, looking for the correct node. Such work duplication is not
> necessary. Instead, once the node is found, cache it, and re-use it.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/spl/spl_fit.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 6/8] image: Do not #if guard board_fit_config_name_match() prototype

[Alexandru Gagniuc]

There's no point in guarding function prototypes with #ifdefs. If a
function is not defined, the linker will notice. Having the prototype
does not affect code size.

What the #if guard takes away is the ability to use IS_ENABLED:

	if (CONFIG_IS ENABLED(FIT_IMAGE_POST_PROCESS))
		board_fit_config_name_match(...)

When the prototype is guarded, the above form cannot be used. This
leads to the proliferation of #ifdefs, and unreadable code. The
opportunity cost of the #if guard outweighs any benefits. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 include/image.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/image.h b/include/image.h
index 00bc03bebe..fd8bd43515 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1536,8 +1536,6 @@ bool android_image_print_dtb_contents(ulong hdr_addr);
  */
 int board_fit_config_name_match(const char *name);
 
-#if defined(CONFIG_SPL_FIT_IMAGE_POST_PROCESS) || \
-	defined(CONFIG_FIT_IMAGE_POST_PROCESS)
 /**
  * board_fit_image_post_process() - Do any post-process on FIT binary data
  *
@@ -1552,7 +1550,6 @@ int board_fit_config_name_match(const char *name);
  * @return no return value (failure should be handled internally)
  */
 void board_fit_image_post_process(void **p_image, size_t *p_size);
-#endif /* CONFIG_SPL_FIT_IMAGE_POST_PROCESS */
 
 #define FDT_ERROR	((ulong)(-1))
 
-- 
2.26.2

[PATCH 6/8] image: Do not #if guard board_fit_config_name_match() prototype

[Simon Glass]

Hi Alexandru

On Tue, 15 Dec 2020 at 17:10, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> There's no point in guarding function prototypes with #ifdefs. If a
> function is not defined, the linker will notice. Having the prototype
> does not affect code size.
>
> What the #if guard takes away is the ability to use IS_ENABLED:
>
>         if (CONFIG_IS ENABLED(FIT_IMAGE_POST_PROCESS))
>                 board_fit_config_name_match(...)
>
> When the prototype is guarded, the above form cannot be used. This
> leads to the proliferation of #ifdefs, and unreadable code. The
> opportunity cost of the #if guard outweighs any benefits. Remove it.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  include/image.h | 3 ---
>  1 file changed, 3 deletions(-)

Yes indeed.

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 7/8] spl: fit: Replace #ifdef blocks with more readable constructs

[Alexandru Gagniuc]

Use the IS_ENABLED() macro to control code flow, instead of the
caveman approach of sprinkling #ifdefs. Code size is not affected, as
the linker garbage-collects unused functions. However, readability is
improved significantly.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl_fit.c | 53 ++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 4b49f369ac..ebfd5fa112 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -297,18 +297,16 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 		src = (void *)data;
 	}
 
-#ifdef CONFIG_SPL_FIT_SIGNATURE
-	printf("## Checking hash(es) for Image %s ... ",
-	       fit_get_name(fit, node, NULL));
-	if (!fit_image_verify_with_data(fit, node,
-					 src, length))
-		return -EPERM;
-	puts("OK\n");
-#endif
+	if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) {
+		printf("## Checking hash(es) for Image %s ... ",
+		       fit_get_name(fit, node, NULL));
+		if (!fit_image_verify_with_data(fit, node, src, length))
+			return -EPERM;
+		puts("OK\n");
+	}
 
-#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
-	board_fit_image_post_process(&src, &length);
-#endif
+	if (CONFIG_IS_ENABLED(FIT_IMAGE_POST_PROCESS))
+		board_fit_image_post_process(&src, &length);
 
 	if (IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == IH_COMP_GZIP) {
 		size = length;
@@ -373,7 +371,9 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 
 	/* Make the load-address of the FDT available for the SPL framework */
 	spl_image->fdt_addr = (void *)image_info.load_addr;
-#if !CONFIG_IS_ENABLED(FIT_IMAGE_TINY)
+	if (CONFIG_IS_ENABLED(FIT_IMAGE_TINY))
+		return 0;
+
 	if (CONFIG_IS_ENABLED(LOAD_FIT_APPLY_OVERLAY)) {
 		void *tmpbuffer = NULL;
 
@@ -431,7 +431,6 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 	ret = fdt_shrink_to_minimum(spl_image->fdt_addr, 8192);
 	if (ret < 0)
 		return ret;
-#endif
 
 	return ret;
 }
@@ -440,10 +439,12 @@ static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index,
 				   void *blob, struct spl_image_info *image)
 {
 	int ret = 0;
-#if !CONFIG_IS_ENABLED(FIT_IMAGE_TINY)
 	const char *name;
 	int node;
 
+	if (CONFIG_IS_ENABLED(FIT_IMAGE_TINY))
+		return 0;
+
 	ret = spl_fit_get_image_name(ctx, "loadables", index, &name);
 	if (ret < 0)
 		return ret;
@@ -454,15 +455,15 @@ static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index,
 				  image->size, image->entry_point,
 				  fdt_getprop(ctx->fit, node, "type", NULL),
 				  fdt_getprop(ctx->fit, node, "os", NULL));
-#endif
 	return ret;
 }
 
 static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os)
 {
-#if CONFIG_IS_ENABLED(FIT_IMAGE_TINY) && !defined(CONFIG_SPL_OS_BOOT)
-	const char *name = fdt_getprop(fit, noffset, FIT_OS_PROP, NULL);
+	if (!CONFIG_IS_ENABLED(FIT_IMAGE_TINY) || CONFIG_IS_ENABLED(OS_BOOT))
+		return fit_image_get_os(fit, noffset, os);
 
+	const char *name = fdt_getprop(fit, noffset, FIT_OS_PROP, NULL);
 	if (!name)
 		return -ENOENT;
 
@@ -477,9 +478,6 @@ static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os)
 		*os = IH_OS_INVALID;
 
 	return 0;
-#else
-	return fit_image_get_os(fit, noffset, os);
-#endif
 }
 
 /*
@@ -629,10 +627,10 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 */
 	if (node < 0)
 		node = spl_fit_get_image_node(&ctx, FIT_FIRMWARE_PROP, 0);
-#ifdef CONFIG_SPL_OS_BOOT
-	if (node < 0)
+
+	if (node < 0 && IS_ENABLED(CONFIG_SPL_OS_BOOT))
 		node = spl_fit_get_image_node(&ctx, FIT_KERNEL_PROP, 0);
-#endif
+
 	if (node < 0) {
 		debug("could not find firmware image, trying loadables...\n");
 		node = spl_fit_get_image_node(&ctx, "loadables", 0);
@@ -659,10 +657,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 */
 	if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os))
 		debug("Image OS is %s\n", genimg_get_os_name(spl_image->os));
-#if !defined(CONFIG_SPL_OS_BOOT)
-	else
+	else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT))
 		spl_image->os = IH_OS_U_BOOT;
-#endif
 
 	/*
 	 * Booting a next-stage U-Boot may require us to append the FDT.
@@ -728,9 +724,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 
 	spl_image->flags |= SPL_FIT_FOUND;
 
-#ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load(ctx.fit);
-#endif
+	if (IS_ENABLED(CONFIG_IMX_HAB))
+		board_spl_fit_post_load(ctx.fit);
 
 	return 0;
 }
-- 
2.26.2

[PATCH 7/8] spl: fit: Replace #ifdef blocks with more readable constructs

[Simon Glass]

On Tue, 15 Dec 2020 at 17:10, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> Use the IS_ENABLED() macro to control code flow, instead of the
> caveman approach of sprinkling #ifdefs. Code size is not affected, as
> the linker garbage-collects unused functions. However, readability is
> improved significantly.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/spl/spl_fit.c | 53 ++++++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 29 deletions(-)

I am trying to imagine stick drawings with #ifdefs

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 7/8] spl: fit: Replace #ifdef blocks with more readable constructs

[Alex G.]

On 12/18/20 8:29 PM, Simon Glass wrote:
> On Tue, 15 Dec 2020 at 17:10, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>
>> Use the IS_ENABLED() macro to control code flow, instead of the
>> caveman approach of sprinkling #ifdefs. Code size is not affected, as
>> the linker garbage-collects unused functions. However, readability is
>> improved significantly.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   common/spl/spl_fit.c | 53 ++++++++++++++++++++------------------------
>>   1 file changed, 24 insertions(+), 29 deletions(-)
> 
> I am trying to imagine stick drawings with #ifdefs
> 
                                    #ifdef
                                  #if     #if
                                #if         #if
                                #if         #if 

                                  #if     #if
                                    #endif
                           #if        #if        #if
                             #if      #if      #if
                               #if    #if    #if
                                 #if  #if  #if
                                    #ifdef
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #endif
                                    #ifdef
                                 #if       #if
                               #if           #if
                             #if               #if
                           #if                   #if
                      #endif                      #endif


> Reviewed-by: Simon Glass <sjg@chromium.org>
> 

[PATCH 7/8] spl: fit: Replace #ifdef blocks with more readable constructs

[Simon Glass]

Hi Alex,

On Mon, 21 Dec 2020 at 10:43, Alex G. <mr.nuke.me@gmail.com> wrote:
>
>
>
> On 12/18/20 8:29 PM, Simon Glass wrote:
> > On Tue, 15 Dec 2020 at 17:10, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
> >>
> >> Use the IS_ENABLED() macro to control code flow, instead of the
> >> caveman approach of sprinkling #ifdefs. Code size is not affected, as
> >> the linker garbage-collects unused functions. However, readability is
> >> improved significantly.
> >>
> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> >> ---
> >>   common/spl/spl_fit.c | 53 ++++++++++++++++++++------------------------
> >>   1 file changed, 24 insertions(+), 29 deletions(-)
> >
> > I am trying to imagine stick drawings with #ifdefs
> >
>                                     #ifdef
>                                   #if     #if
>                                 #if         #if
>                                 #if         #if
>
>                                   #if     #if
>                                     #endif
>                            #if        #if        #if
>                              #if      #if      #if
>                                #if    #if    #if
>                                  #if  #if  #if
>                                     #ifdef
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #endif
>                                     #ifdef
>                                  #if       #if
>                                #if           #if
>                              #if               #if
>                            #if                   #if
>                       #endif                      #endif
>

So you are an artist and an engineer.

Regards,
Simon

[PATCH 8/8] spl: fit: Load devicetree when a Linux payload is found

[Alexandru Gagniuc]

When a FIT config specifies a devicetree, we should load it, no
questions asked. In the case of the "simple" FIT loading path, a
difficulty arises in selecting the load address of the FDT.

The default FDT location is right after the "kernel" or "firmware"
image. However, if that is an OP-TEE image, then the FDT may end up in
secure DRAM, and not be accessible to normal world kernels.

Although the best solution is to be more careful about the FDT
address, a viable workaround is to only append the FDT after a u-boot
or Linux image. This is identical to the previous logic, except that
FDT loading is extended to IH_OS_LINUX images.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl_fit.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index ebfd5fa112..e64fde9e86 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -335,6 +335,18 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 	return 0;
 }
 
+static bool os_takes_devicetree(uint8_t os)
+{
+	switch (os) {
+	case IH_OS_U_BOOT:
+		return true;
+	case IH_OS_LINUX:
+		return IS_ENABLED(CONFIG_SPL_OS_BOOT);
+	default:
+		return false;
+	}
+}
+
 static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 			      struct spl_load_info *info, ulong sector,
 			      const struct spl_fit_info *ctx)
@@ -664,9 +676,9 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * Booting a next-stage U-Boot may require us to append the FDT.
 	 * We allow this to fail, as the U-Boot image might embed its FDT.
 	 */
-	if (spl_image->os == IH_OS_U_BOOT) {
+	if (os_takes_devicetree(spl_image->os)) {
 		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
-		if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0)
+		if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
 			return ret;
 	}
 
@@ -694,7 +706,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 		if (!spl_fit_image_get_os(ctx.fit, node, &os_type))
 			debug("Loadable is %s\n", genimg_get_os_name(os_type));
 
-		if (os_type == IH_OS_U_BOOT) {
+		if (os_takes_devicetree(spl_image->os)) {
 			spl_fit_append_fdt(&image_info, info, sector, &ctx);
 			spl_image->fdt_addr = image_info.fdt_addr;
 		}
-- 
2.26.2

[PATCH 8/8] spl: fit: Load devicetree when a Linux payload is found

[Alex G.]

On 12/15/20 6:09 PM, Alexandru Gagniuc wrote:
> When a FIT config specifies a devicetree, we should load it, no
> questions asked. In the case of the "simple" FIT loading path, a
> difficulty arises in selecting the load address of the FDT.
> 
> The default FDT location is right after the "kernel" or "firmware"
> image. However, if that is an OP-TEE image, then the FDT may end up in
> secure DRAM, and not be accessible to normal world kernels.
> 
> Although the best solution is to be more careful about the FDT
> address, a viable workaround is to only append the FDT after a u-boot
> or Linux image. This is identical to the previous logic, except that
> FDT loading is extended to IH_OS_LINUX images.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   common/spl/spl_fit.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index ebfd5fa112..e64fde9e86 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -335,6 +335,18 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
>   	return 0;
>   }
>   
> +static bool os_takes_devicetree(uint8_t os)
> +{
> +	switch (os) {
> +	case IH_OS_U_BOOT:
> +		return true;
> +	case IH_OS_LINUX:
> +		return IS_ENABLED(CONFIG_SPL_OS_BOOT);
> +	default:
> +		return false;
> +	}
> +}
> +
>   static int spl_fit_append_fdt(struct spl_image_info *spl_image,
>   			      struct spl_load_info *info, ulong sector,
>   			      const struct spl_fit_info *ctx)
> @@ -664,9 +676,9 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>   	 * Booting a next-stage U-Boot may require us to append the FDT.
>   	 * We allow this to fail, as the U-Boot image might embed its FDT.
>   	 */
> -	if (spl_image->os == IH_OS_U_BOOT) {
> +	if (os_takes_devicetree(spl_image->os)) {

The if clause here should say "if (os_takes_devicetree(os_type)) {". I 
will fix this in v2.

>   		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
> -		if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0)
> +		if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
>   			return ret;
>   	}
>   
> @@ -694,7 +706,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>   		if (!spl_fit_image_get_os(ctx.fit, node, &os_type))
>   			debug("Loadable is %s\n", genimg_get_os_name(os_type));
>   
> -		if (os_type == IH_OS_U_BOOT) {
> +		if (os_takes_devicetree(spl_image->os)) {
>   			spl_fit_append_fdt(&image_info, info, sector, &ctx);
>   			spl_image->fdt_addr = image_info.fdt_addr;
>   		}
> 

[PATCH v2 0/8] spl: fit: Play nicely with OP-TEE and Linux

[Alexandru Gagniuc]

This patch series is part of a larger effort to get linux to boot
really fast alongside a secure OS. One piece of the puzzle is getting
Linux and OP-TEE to boot straight from SPL. This is where the FIT
image comes in.

The "simple" fit code was mostly ready for this, although it was
quite difficult to navigate. As I was figuring out the required
changes, I realized I had also managed to do some major refactoring.
In fact, of the eight changes, (6) are refactoring patches, and (2)
are functional changes.

I wish I could have written this with a negative line count. I have
unfortunately failed at that, and this series will be adding 11 lines
to u-boot. The takeaway is that the following type FIT can be loaded
from SPL directly:

        images {
                optee at 1 {
                        type = "tee";
                        os = "tee";
                        ...
                };
                kernel at 1 {
                        type = "kernel";
                        os = "linux";
			...
                };
                fdt at devicetree.dtb { ... }
                fdt at overlay.dto { ... };
        };

        configurations {
                conf at quickboot {
                        kernel = "optee at 1";
                        fdt = "fdt at stm32mp157c-ev1.dtb", "fdt at overlay.dto";
                        loadables = "kernel at 1";
                };
        };


This series is designed to apply to u-boot/next:
  * 8351a29d2d Merge tag 'dm-pull-14dec20' of git://git.denx.de/u-boot-dm into next	
  
Changes since v1:
 * Added struct comments (Simon Glass)
 * Added comment do describe args of board_spl_fit_post_load() (Simon Glass)
 * Fixed predicate of if clause on spl_load_simple_fit() (me)
 
Simon requested that I write a test case to check the simple SPL FIT
loading path. I looked at it, and I couldn't quite figure out how to
connect the "sandbox" to a python test. Although the idea is great in
principle, it this seems to be a longer side-project that is beyond
the scope of this series.

Alexandru Gagniuc (8):
  spl: fit: Drop 'length' argument to board_spl_fit_post_load()
  spl: fit: Factor out FIT parsing and use a context struct
  spl: fit: Pass FIT context via a structure pointer
  spl: fit: Remove useless loop in spl_fit_get_image_name()
  spl: fit: Only look up FIT configuration node once
  image: Do not #if guard board_fit_config_name_match() prototype
  spl: fit: Replace #ifdef blocks with more readable constructs
  spl: fit: Load devicetree when a Linux payload is found

 arch/arm/mach-imx/spl.c |   5 +-
 common/spl/spl_fit.c    | 257 +++++++++++++++++++++-------------------
 include/image.h         |   3 -
 include/spl.h           |   4 +-
 4 files changed, 140 insertions(+), 129 deletions(-)

-- 
2.26.2

[PATCH v3 0/8] spl: fit: Play nicely with OP-TEE and Linux

[Alexandru Gagniuc]

This patch series is part of a larger effort to get linux to boot
really fast alongside a secure OS. One piece of the puzzle is getting
Linux and OP-TEE to boot straight from SPL. This is where the FIT
image comes in.

The "simple" fit code was mostly ready for this, although it was
quite difficult to navigate. As I was figuring out the required
changes, I realized I had also managed to do some major refactoring.
In fact, of the eight changes, (6) are refactoring patches, and (2)
are functional changes.

I wish I could have written this with a negative line count. I have
unfortunately failed at that, and this series will be adding 11 lines
to u-boot. The takeaway is that the following type FIT can be loaded
from SPL directly:

        images {
                optee at 1 {
                        type = "tee";
                        os = "tee";
                        ...
                };
                kernel at 1 {
                        type = "kernel";
                        os = "linux";
			...
                };
                fdt at devicetree.dtb { ... }
                fdt at overlay.dto { ... };
        };

        configurations {
                conf at quickboot {
                        kernel = "optee at 1";
                        fdt = "fdt at stm32mp157c-ev1.dtb", "fdt at overlay.dto";
                        loadables = "kernel at 1";
                };
        };


This series is designed to apply to u-boot/next:
  * 8351a29d2d Merge tag 'dm-pull-14dec20' of git://git.denx.de/u-boot-dm into next	
  
Changes since v1:
 * Added struct comments (Simon Glass)
 * Added comment do describe args of board_spl_fit_post_load() (Simon Glass)
 * Fixed predicate of if clause on spl_load_simple_fit() (me)
 
Changes since v2:
 * Fixed embarrasing rebase mishap on one of the patches.
 

Alexandru Gagniuc (8):
  spl: fit: Drop 'length' argument to board_spl_fit_post_load()
  spl: fit: Factor out FIT parsing and use a context struct
  spl: fit: Pass FIT context via a structure pointer
  spl: fit: Remove useless loop in spl_fit_get_image_name()
  spl: fit: Only look up FIT configuration node once
  image: Do not #if guard board_fit_config_name_match() prototype
  spl: fit: Replace #ifdef blocks with more readable constructs
  spl: fit: Load devicetree when a Linux payload is found

 arch/arm/mach-imx/spl.c |   5 +-
 common/spl/spl_fit.c    | 261 +++++++++++++++++++++-------------------
 include/image.h         |   3 -
 include/spl.h           |   4 +-
 4 files changed, 141 insertions(+), 132 deletions(-)

-- 
2.26.2

[PATCH v3 1/8] spl: fit: Drop 'length' argument to board_spl_fit_post_load()

[Alexandru Gagniuc]

The size is derived from the FIT image itself. Any alignment
requirements are machine-specific and known by the board code. Thus
the total length can be derived from the FIT image and knowledge of
the platform. The 'length' argument is redundant. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/mach-imx/spl.c | 5 +++--
 common/spl/spl_fit.c    | 4 ++--
 include/spl.h           | 4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
index aa2686bb92..11255798d3 100644
--- a/arch/arm/mach-imx/spl.c
+++ b/arch/arm/mach-imx/spl.c
@@ -18,6 +18,7 @@
 #include <asm/mach-imx/hab.h>
 #include <asm/mach-imx/boot_mode.h>
 #include <g_dnl.h>
+#include <linux/libfdt.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -318,9 +319,9 @@ ulong board_spl_fit_size_align(ulong size)
 	return size;
 }
 
-void board_spl_fit_post_load(ulong load_addr, size_t length)
+void board_spl_fit_post_load(const void *fit)
 {
-	u32 offset = length - CONFIG_CSF_SIZE;
+	u32 offset = ALIGN(fdt_totalsize(fit), 0x1000);
 
 	if (imx_hab_authenticate_image(load_addr,
 				       offset + IVT_SIZE + CSF_PAD_SIZE,
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 795e2922ce..1b4a7f6b15 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -26,7 +26,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
 #endif
 
-__weak void board_spl_fit_post_load(ulong load_addr, size_t length)
+__weak void board_spl_fit_post_load(const void *fit)
 {
 }
 
@@ -722,7 +722,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	spl_image->flags |= SPL_FIT_FOUND;
 
 #ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load((ulong)fit, size);
+	board_spl_fit_post_load(fit);
 #endif
 
 	return 0;
diff --git a/include/spl.h b/include/spl.h
index 374a295fa3..cb30ef0827 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -630,9 +630,9 @@ int board_return_to_bootrom(struct spl_image_info *spl_image,
 
 /**
  * board_spl_fit_post_load - allow process images after loading finished
- *
+ * @fit: Pointer to a valid Flattened Image Tree blob
  */
-void board_spl_fit_post_load(ulong load_addr, size_t length);
+void board_spl_fit_post_load(const void *fit);
 
 /**
  * board_spl_fit_size_align - specific size align before processing payload
-- 
2.26.2

[PATCH v3 1/8] spl: fit: Drop 'length' argument to board_spl_fit_post_load()

[Tom Rini]

On Wed, Dec 23, 2020 at 08:44:05AM -0600, Alexandru Gagniuc wrote:

> The size is derived from the FIT image itself. Any alignment
> requirements are machine-specific and known by the board code. Thus
> the total length can be derived from the FIT image and knowledge of
> the platform. The 'length' argument is redundant. Remove it.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  arch/arm/mach-imx/spl.c | 5 +++--
>  common/spl/spl_fit.c    | 4 ++--
>  include/spl.h           | 4 ++--
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
> index aa2686bb92..11255798d3 100644
> --- a/arch/arm/mach-imx/spl.c
> +++ b/arch/arm/mach-imx/spl.c
> @@ -18,6 +18,7 @@
>  #include <asm/mach-imx/hab.h>
>  #include <asm/mach-imx/boot_mode.h>
>  #include <g_dnl.h>
> +#include <linux/libfdt.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -318,9 +319,9 @@ ulong board_spl_fit_size_align(ulong size)
>  	return size;
>  }
>  
> -void board_spl_fit_post_load(ulong load_addr, size_t length)
> +void board_spl_fit_post_load(const void *fit)
>  {
> -	u32 offset = length - CONFIG_CSF_SIZE;
> +	u32 offset = ALIGN(fdt_totalsize(fit), 0x1000);
>  
>  	if (imx_hab_authenticate_image(load_addr,
>  				       offset + IVT_SIZE + CSF_PAD_SIZE,

OK, this is a problem right here.  First, the code no longer compiles as
we don't pass in "load_addr", which is what
imx_hab_authenticate_image() takes to know where in DDR the image is to
validate.  While I could probably work out that value from what we use
now for offset, I would rather someone that can test the code do so.
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210115/79b4a3f8/attachment.sig>

[PATCH v3 1/8] spl: fit: Drop 'length' argument to board_spl_fit_post_load()

[Alex G.]

On 1/15/21 8:33 PM, Tom Rini wrote:
> On Wed, Dec 23, 2020 at 08:44:05AM -0600, Alexandru Gagniuc wrote:
> 
>> The size is derived from the FIT image itself. Any alignment
>> requirements are machine-specific and known by the board code. Thus
>> the total length can be derived from the FIT image and knowledge of
>> the platform. The 'length' argument is redundant. Remove it.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> Reviewed-by: Peng Fan <peng.fan@nxp.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> ---
>>   arch/arm/mach-imx/spl.c | 5 +++--
>>   common/spl/spl_fit.c    | 4 ++--
>>   include/spl.h           | 4 ++--
>>   3 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
>> index aa2686bb92..11255798d3 100644
>> --- a/arch/arm/mach-imx/spl.c
>> +++ b/arch/arm/mach-imx/spl.c
>> @@ -18,6 +18,7 @@
>>   #include <asm/mach-imx/hab.h>
>>   #include <asm/mach-imx/boot_mode.h>
>>   #include <g_dnl.h>
>> +#include <linux/libfdt.h>
>>   
>>   DECLARE_GLOBAL_DATA_PTR;
>>   
>> @@ -318,9 +319,9 @@ ulong board_spl_fit_size_align(ulong size)
>>   	return size;
>>   }
>>   
>> -void board_spl_fit_post_load(ulong load_addr, size_t length)
>> +void board_spl_fit_post_load(const void *fit)
>>   {
>> -	u32 offset = length - CONFIG_CSF_SIZE;
>> +	u32 offset = ALIGN(fdt_totalsize(fit), 0x1000);
>>   
>>   	if (imx_hab_authenticate_image(load_addr,
>>   				       offset + IVT_SIZE + CSF_PAD_SIZE,
> 
> OK, this is a problem right here.  First, the code no longer compiles as
> we don't pass in "load_addr", which is what
> imx_hab_authenticate_image() takes to know where in DDR the image is to
> validate.  While I could probably work out that value from what we use
> now for offset, I would rather someone that can test the code do so.
> Thanks!
>

Hi Tom,

I'm sorry I missed that. I seemed to have, again, solved the hard 
problem and choked on something simple. Being able to eliminate the 
'length' argument is essential simplifying the FIT code, and the rest of 
this series. Fixing the compilation issue is trivial, but how do we get 
this tested? Do you know someone with the hardware who'd be willing to 
give it a shot?

Alex

[PATCH v3 1/8] spl: fit: Drop 'length' argument to board_spl_fit_post_load()

[Tom Rini]

On Mon, Jan 18, 2021 at 10:28:45AM -0600, Alex G. wrote:
> On 1/15/21 8:33 PM, Tom Rini wrote:
> > On Wed, Dec 23, 2020 at 08:44:05AM -0600, Alexandru Gagniuc wrote:
> > 
> > > The size is derived from the FIT image itself. Any alignment
> > > requirements are machine-specific and known by the board code. Thus
> > > the total length can be derived from the FIT image and knowledge of
> > > the platform. The 'length' argument is redundant. Remove it.
> > > 
> > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> > > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >   arch/arm/mach-imx/spl.c | 5 +++--
> > >   common/spl/spl_fit.c    | 4 ++--
> > >   include/spl.h           | 4 ++--
> > >   3 files changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
> > > index aa2686bb92..11255798d3 100644
> > > --- a/arch/arm/mach-imx/spl.c
> > > +++ b/arch/arm/mach-imx/spl.c
> > > @@ -18,6 +18,7 @@
> > >   #include <asm/mach-imx/hab.h>
> > >   #include <asm/mach-imx/boot_mode.h>
> > >   #include <g_dnl.h>
> > > +#include <linux/libfdt.h>
> > >   DECLARE_GLOBAL_DATA_PTR;
> > > @@ -318,9 +319,9 @@ ulong board_spl_fit_size_align(ulong size)
> > >   	return size;
> > >   }
> > > -void board_spl_fit_post_load(ulong load_addr, size_t length)
> > > +void board_spl_fit_post_load(const void *fit)
> > >   {
> > > -	u32 offset = length - CONFIG_CSF_SIZE;
> > > +	u32 offset = ALIGN(fdt_totalsize(fit), 0x1000);
> > >   	if (imx_hab_authenticate_image(load_addr,
> > >   				       offset + IVT_SIZE + CSF_PAD_SIZE,
> > 
> > OK, this is a problem right here.  First, the code no longer compiles as
> > we don't pass in "load_addr", which is what
> > imx_hab_authenticate_image() takes to know where in DDR the image is to
> > validate.  While I could probably work out that value from what we use
> > now for offset, I would rather someone that can test the code do so.
> > Thanks!
> > 
> 
> Hi Tom,
> 
> I'm sorry I missed that. I seemed to have, again, solved the hard problem
> and choked on something simple. Being able to eliminate the 'length'
> argument is essential simplifying the FIT code, and the rest of this series.
> Fixing the compilation issue is trivial, but how do we get this tested? Do
> you know someone with the hardware who'd be willing to give it a shot?

Actually, yes.  Please cc Matt Porter on the next re-spin and he can run
a boot test on fused hardware.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210118/ad7ccbb2/attachment.sig>

[PATCH v3 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Alexandru Gagniuc]

The logical steps in spl_load_simple_fit() are difficult to follow.
I think the long comments, ifdefs, and ungodly number of variables
seriously affect the readability. In particular, it violates section 6
of the coding style, paragraphs (3), and (4).

The purpose of this patch is to improve the situation by
  - Factoring out initialization and parsing to separate functions
  - Reduce the number of variables by using a context structure
This change introduces no functional changes.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl_fit.c | 90 +++++++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 30 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 1b4a7f6b15..bcc943b0b8 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -26,6 +26,12 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
 #endif
 
+struct spl_fit_info {
+	const void *fit;	/* Pointer to a valid FIT blob */
+	size_t ext_data_offset;	/* Offset to FIT external data (end of FIT) */
+	int images_node;	/* FDT offset to "/images" node */
+};
+
 __weak void board_spl_fit_post_load(const void *fit)
 {
 }
@@ -521,28 +527,22 @@ __weak bool spl_load_simple_fit_skip_processing(void)
 	return false;
 }
 
-int spl_load_simple_fit(struct spl_image_info *spl_image,
-			struct spl_load_info *info, ulong sector, void *fit)
+static int spl_simple_fit_read(struct spl_fit_info *ctx,
+			       struct spl_load_info *info, ulong sector,
+			       const void *fit_header)
 {
+	unsigned long count, size;
 	int sectors;
-	ulong size, hsize;
-	unsigned long count;
-	struct spl_image_info image_info;
-	int node = -1;
-	int images, ret;
-	int base_offset;
-	int index = 0;
-	int firmware_node;
+	void *buf;
 
 	/*
 	 * For FIT with external data, figure out where the external images
 	 * start. This is the base for the data-offset properties in each
 	 * image.
 	 */
-	size = fdt_totalsize(fit);
-	size = (size + 3) & ~3;
+	size = ALIGN(fdt_totalsize(fit_header), 4);
 	size = board_spl_fit_size_align(size);
-	base_offset = (size + 3) & ~3;
+	ctx->ext_data_offset = ALIGN(size, 4);
 
 	/*
 	 * So far we only have one block of data from the FIT. Read the entire
@@ -552,36 +552,66 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * For FIT with external data, data is not loaded in this step.
 	 */
 	sectors = get_aligned_image_size(info, size, 0);
-	hsize = sectors * info->bl_len;
-	fit = spl_get_fit_load_buffer(hsize);
-	count = info->read(info, sector, sectors, fit);
-	debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu, size=0x%lx\n",
-	      sector, sectors, fit, count, size);
+	buf = spl_get_fit_load_buffer(sectors * info->bl_len);
 
-	if (count == 0)
-		return -EIO;
+	count = info->read(info, sector, sectors, buf);
+	ctx->fit = buf;
+	debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu, size=0x%lx\n",
+	      sector, sectors, buf, count, size);
 
-	/* skip further processing if requested to enable load-only use cases */
-	if (spl_load_simple_fit_skip_processing())
-		return 0;
+	return (count == 0) ? -EIO : 0;
+}
 
+static int spl_simple_fit_parse(struct spl_fit_info *ctx)
+{
 	if (IS_ENABLED(CONFIG_SPL_FIT_SIGNATURE)) {
-		int conf_offset = fit_find_config_node(fit);
+		int conf_offset = fit_find_config_node(ctx->fit);
 
 		printf("## Checking hash(es) for config %s ... ",
-		       fit_get_name(fit, conf_offset, NULL));
-		if (fit_config_verify(fit, conf_offset))
+		       fit_get_name(ctx->fit, conf_offset, NULL));
+		if (fit_config_verify(ctx->fit, conf_offset))
 			return -EPERM;
 		puts("OK\n");
 	}
 
 	/* find the node holding the images information */
-	images = fdt_path_offset(fit, FIT_IMAGES_PATH);
-	if (images < 0) {
-		debug("%s: Cannot find /images node: %d\n", __func__, images);
-		return -1;
+	ctx->images_node = fdt_path_offset(ctx->fit, FIT_IMAGES_PATH);
+	if (ctx->images_node < 0) {
+		debug("%s: Cannot find /images node: %d\n", __func__,
+		      ctx->images_node);
+		return -EINVAL;
 	}
 
+	return 0;
+}
+
+int spl_load_simple_fit(struct spl_image_info *spl_image,
+			struct spl_load_info *info, ulong sector, void *fit)
+{
+	struct spl_image_info image_info;
+	struct spl_fit_info ctx;
+	int node = -1;
+	int images, ret;
+	int base_offset;
+	int index = 0;
+	int firmware_node;
+
+	ret = spl_simple_fit_read(&ctx, info, sector, fit);
+	if (ret < 0)
+		return ret;
+
+	/* skip further processing if requested to enable load-only use cases */
+	if (spl_load_simple_fit_skip_processing())
+		return 0;
+
+	ret = spl_simple_fit_parse(&ctx);
+	if (ret < 0)
+		return ret;
+
+	images = ctx.images_node;
+	fit = (void *)ctx.fit;
+	base_offset = ctx.ext_data_offset;
+
 #ifdef CONFIG_SPL_FPGA
 	node = spl_fit_get_image_node(fit, images, "fpga", 0);
 	if (node >= 0) {
-- 
2.26.2

[PATCH v3 3/8] spl: fit: Pass FIT context via a structure pointer

[Alexandru Gagniuc]

Several loose arguments describe the FIT image. They are thus related,
and it makes sense to pass them together, in a structure. Examples
include the FIT blob pointer, offset to FDT nodes, and the offset to
external data.

Use a spl_fit_info structure to group these parameters.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/spl/spl_fit.c | 101 ++++++++++++++++++-------------------------
 1 file changed, 43 insertions(+), 58 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index bcc943b0b8..5a06e8a9db 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -76,7 +76,7 @@ static int find_node_from_desc(const void *fit, int node, const char *str)
  *
  * Return:	0 on success, or a negative error number
  */
-static int spl_fit_get_image_name(const void *fit, int images,
+static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
 				  const char *type, int index,
 				  const char **outname)
 {
@@ -87,21 +87,21 @@ static int spl_fit_get_image_name(const void *fit, int images,
 	int len, i;
 	bool found = true;
 
-	conf_node = fit_find_config_node(fit);
+	conf_node = fit_find_config_node(ctx->fit);
 	if (conf_node < 0) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
 		printf("No matching DT out of these options:\n");
-		for (node = fdt_first_subnode(fit, conf_node);
+		for (node = fdt_first_subnode(ctx->fit, conf_node);
 		     node >= 0;
-		     node = fdt_next_subnode(fit, node)) {
-			name = fdt_getprop(fit, node, "description", &len);
+		     node = fdt_next_subnode(ctx->fit, node)) {
+			name = fdt_getprop(ctx->fit, node, "description", &len);
 			printf("   %s\n", name);
 		}
 #endif
 		return conf_node;
 	}
 
-	name = fdt_getprop(fit, conf_node, type, &len);
+	name = fdt_getprop(ctx->fit, conf_node, type, &len);
 	if (!name) {
 		debug("cannot find property '%s': %d\n", type, len);
 		return -EINVAL;
@@ -135,11 +135,11 @@ static int spl_fit_get_image_name(const void *fit, int images,
 			 * node name.
 			 */
 			int node;
-			int images = fdt_path_offset(fit, FIT_IMAGES_PATH);
+			int images = fdt_path_offset(ctx->fit, FIT_IMAGES_PATH);
 
-			node = find_node_from_desc(fit, images, str);
+			node = find_node_from_desc(ctx->fit, images, str);
 			if (node > 0)
-				str = fdt_get_name(fit, node, NULL);
+				str = fdt_get_name(ctx->fit, node, NULL);
 
 			found = true;
 		}
@@ -166,20 +166,20 @@ static int spl_fit_get_image_name(const void *fit, int images,
  * Return:	the node offset of the respective image node or a negative
  *		error number.
  */
-static int spl_fit_get_image_node(const void *fit, int images,
+static int spl_fit_get_image_node(const struct spl_fit_info *ctx,
 				  const char *type, int index)
 {
 	const char *str;
 	int err;
 	int node;
 
-	err = spl_fit_get_image_name(fit, images, type, index, &str);
+	err = spl_fit_get_image_name(ctx, type, index, &str);
 	if (err)
 		return err;
 
 	debug("%s: '%s'\n", type, str);
 
-	node = fdt_subnode_offset(fit, images, str);
+	node = fdt_subnode_offset(ctx->fit, ctx->images_node, str);
 	if (node < 0) {
 		pr_err("cannot find image node '%s': %d\n", str, node);
 		return -EINVAL;
@@ -230,10 +230,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
  * spl_load_fit_image(): load the image described in a certain FIT node
  * @info:	points to information about the device to load data from
  * @sector:	the start sector of the FIT image on the device
- * @fit:	points to the flattened device tree blob describing the FIT
- *		image
- * @base_offset: the beginning of the data area containing the actual
- *		image data, relative to the beginning of the FIT
+ * @ctx:	points to the FIT context structure
  * @node:	offset of the DT node describing the image to load (relative
  *		to @fit)
  * @image_info:	will be filled with information about the loaded image
@@ -244,7 +241,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
  * Return:	0 on success or a negative error number.
  */
 static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
-			      void *fit, ulong base_offset, int node,
+			      const struct spl_fit_info *ctx, int node,
 			      struct spl_image_info *image_info)
 {
 	int offset;
@@ -258,6 +255,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 	int align_len = ARCH_DMA_MINALIGN - 1;
 	uint8_t image_comp = -1, type = -1;
 	const void *data;
+	const void *fit = ctx->fit;
 	bool external_data = false;
 
 	if (IS_ENABLED(CONFIG_SPL_FPGA) ||
@@ -279,7 +277,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 	if (!fit_image_get_data_position(fit, node, &offset)) {
 		external_data = true;
 	} else if (!fit_image_get_data_offset(fit, node, &offset)) {
-		offset += base_offset;
+		offset += ctx->ext_data_offset;
 		external_data = true;
 	}
 
@@ -355,7 +353,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 
 static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 			      struct spl_load_info *info, ulong sector,
-			      void *fit, int images, ulong base_offset)
+			      const struct spl_fit_info *ctx)
 {
 	struct spl_image_info image_info;
 	int node, ret = 0, index = 0;
@@ -367,7 +365,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 	image_info.load_addr = spl_image->load_addr + spl_image->size;
 
 	/* Figure out which device tree the board wants to use */
-	node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, index++);
+	node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index++);
 	if (node < 0) {
 		debug("%s: cannot find FDT node\n", __func__);
 
@@ -381,7 +379,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 		else
 			return node;
 	} else {
-		ret = spl_load_fit_image(info, sector, fit, base_offset, node,
+		ret = spl_load_fit_image(info, sector, ctx, node,
 					 &image_info);
 		if (ret < 0)
 			return ret;
@@ -394,8 +392,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 		void *tmpbuffer = NULL;
 
 		for (; ; index++) {
-			node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP,
-						      index);
+			node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index);
 			if (node == -E2BIG) {
 				debug("%s: No additional FDT node\n", __func__);
 				break;
@@ -418,7 +415,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 					      __func__);
 			}
 			image_info.load_addr = (ulong)tmpbuffer;
-			ret = spl_load_fit_image(info, sector, fit, base_offset,
+			ret = spl_load_fit_image(info, sector, ctx,
 						 node, &image_info);
 			if (ret < 0)
 				break;
@@ -433,12 +430,12 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 							(void *)image_info.load_addr);
 			if (ret) {
 				pr_err("failed to apply DT overlay %s\n",
-				       fit_get_name(fit, node, NULL));
+				       fit_get_name(ctx->fit, node, NULL));
 				break;
 			}
 
 			debug("%s: DT overlay %s applied\n", __func__,
-			      fit_get_name(fit, node, NULL));
+			      fit_get_name(ctx->fit, node, NULL));
 		}
 		free(tmpbuffer);
 		if (ret)
@@ -453,7 +450,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 	return ret;
 }
 
-static int spl_fit_record_loadable(const void *fit, int images, int index,
+static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index,
 				   void *blob, struct spl_image_info *image)
 {
 	int ret = 0;
@@ -461,17 +458,16 @@ static int spl_fit_record_loadable(const void *fit, int images, int index,
 	const char *name;
 	int node;
 
-	ret = spl_fit_get_image_name(fit, images, "loadables",
-				     index, &name);
+	ret = spl_fit_get_image_name(ctx, "loadables", index, &name);
 	if (ret < 0)
 		return ret;
 
-	node = spl_fit_get_image_node(fit, images, "loadables", index);
+	node = spl_fit_get_image_node(ctx, "loadables", index);
 
 	ret = fdt_record_loadable(blob, index, name, image->load_addr,
 				  image->size, image->entry_point,
-				  fdt_getprop(fit, node, "type", NULL),
-				  fdt_getprop(fit, node, "os", NULL));
+				  fdt_getprop(ctx->fit, node, "type", NULL),
+				  fdt_getprop(ctx->fit, node, "os", NULL));
 #endif
 	return ret;
 }
@@ -591,8 +587,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	struct spl_image_info image_info;
 	struct spl_fit_info ctx;
 	int node = -1;
-	int images, ret;
-	int base_offset;
+	int ret;
 	int index = 0;
 	int firmware_node;
 
@@ -608,16 +603,11 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	if (ret < 0)
 		return ret;
 
-	images = ctx.images_node;
-	fit = (void *)ctx.fit;
-	base_offset = ctx.ext_data_offset;
-
 #ifdef CONFIG_SPL_FPGA
-	node = spl_fit_get_image_node(fit, images, "fpga", 0);
+	node = spl_fit_get_image_node(&ctx, "fpga", 0);
 	if (node >= 0) {
 		/* Load the image and set up the spl_image structure */
-		ret = spl_load_fit_image(info, sector, fit, base_offset, node,
-					 spl_image);
+		ret = spl_load_fit_image(info, sector, ctx, node, spl_image);
 		if (ret) {
 			printf("%s: Cannot load the FPGA: %i\n", __func__, ret);
 			return ret;
@@ -646,15 +636,14 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 *   - fall back to using the first 'loadables' entry
 	 */
 	if (node < 0)
-		node = spl_fit_get_image_node(fit, images, FIT_FIRMWARE_PROP,
-					      0);
+		node = spl_fit_get_image_node(&ctx, FIT_FIRMWARE_PROP, 0);
 #ifdef CONFIG_SPL_OS_BOOT
 	if (node < 0)
-		node = spl_fit_get_image_node(fit, images, FIT_KERNEL_PROP, 0);
+		node = spl_fit_get_image_node(&ctx, FIT_KERNEL_PROP, 0);
 #endif
 	if (node < 0) {
 		debug("could not find firmware image, trying loadables...\n");
-		node = spl_fit_get_image_node(fit, images, "loadables", 0);
+		node = spl_fit_get_image_node(&ctx, "loadables", 0);
 		/*
 		 * If we pick the U-Boot image from "loadables", start at
 		 * the second image when later loading additional images.
@@ -668,8 +657,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	}
 
 	/* Load the image and set up the spl_image structure */
-	ret = spl_load_fit_image(info, sector, fit, base_offset, node,
-				 spl_image);
+	ret = spl_load_fit_image(info, sector, &ctx, node, spl_image);
 	if (ret)
 		return ret;
 
@@ -677,7 +665,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * For backward compatibility, we treat the first node that is
 	 * as a U-Boot image, if no OS-type has been declared.
 	 */
-	if (!spl_fit_image_get_os(fit, node, &spl_image->os))
+	if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os))
 		debug("Image OS is %s\n", genimg_get_os_name(spl_image->os));
 #if !defined(CONFIG_SPL_OS_BOOT)
 	else
@@ -689,8 +677,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * We allow this to fail, as the U-Boot image might embed its FDT.
 	 */
 	if (spl_image->os == IH_OS_U_BOOT) {
-		ret = spl_fit_append_fdt(spl_image, info, sector, fit,
-					 images, base_offset);
+		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
 		if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0)
 			return ret;
 	}
@@ -700,7 +687,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	for (; ; index++) {
 		uint8_t os_type = IH_OS_INVALID;
 
-		node = spl_fit_get_image_node(fit, images, "loadables", index);
+		node = spl_fit_get_image_node(&ctx, "loadables", index);
 		if (node < 0)
 			break;
 
@@ -712,17 +699,15 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 		if (firmware_node == node)
 			continue;
 
-		ret = spl_load_fit_image(info, sector, fit, base_offset, node,
-					 &image_info);
+		ret = spl_load_fit_image(info, sector, &ctx, node, &image_info);
 		if (ret < 0)
 			continue;
 
-		if (!spl_fit_image_get_os(fit, node, &os_type))
+		if (!spl_fit_image_get_os(ctx.fit, node, &os_type))
 			debug("Loadable is %s\n", genimg_get_os_name(os_type));
 
 		if (os_type == IH_OS_U_BOOT) {
-			spl_fit_append_fdt(&image_info, info, sector,
-					   fit, images, base_offset);
+			spl_fit_append_fdt(&image_info, info, sector, &ctx);
 			spl_image->fdt_addr = image_info.fdt_addr;
 		}
 
@@ -736,7 +721,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 
 		/* Record our loadables into the FDT */
 		if (spl_image->fdt_addr)
-			spl_fit_record_loadable(fit, images, index,
+			spl_fit_record_loadable(&ctx, index,
 						spl_image->fdt_addr,
 						&image_info);
 	}
@@ -752,7 +737,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	spl_image->flags |= SPL_FIT_FOUND;
 
 #ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load(fit);
+	board_spl_fit_post_load(ctx.fit);
 #endif
 
 	return 0;
-- 
2.26.2

[PATCH v3 4/8] spl: fit: Remove useless loop in spl_fit_get_image_name()

[Alexandru Gagniuc]

When a desired configuration is not found, conf_node will have a
negative value. Thus the for loop will start at the root "/" node of
the image, print the "/description" property, and stop.

It appears the intent of the loop was to print the names of the
subnodes under "/configurations". We would need the offset to the
"/configurations" node, which is abstracted by fit_find_config_node().

This change agrees that abstracting the node offset is the correct
design, and we shouldn't be parsing the configurations manually. Thus
the loop in spl_fit_get_image_name() is useless. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/spl/spl_fit.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 5a06e8a9db..adcd6e3fcd 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -88,18 +88,8 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
 	bool found = true;
 
 	conf_node = fit_find_config_node(ctx->fit);
-	if (conf_node < 0) {
-#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-		printf("No matching DT out of these options:\n");
-		for (node = fdt_first_subnode(ctx->fit, conf_node);
-		     node >= 0;
-		     node = fdt_next_subnode(ctx->fit, node)) {
-			name = fdt_getprop(ctx->fit, node, "description", &len);
-			printf("   %s\n", name);
-		}
-#endif
+	if (conf_node < 0)
 		return conf_node;
-	}
 
 	name = fdt_getprop(ctx->fit, conf_node, type, &len);
 	if (!name) {
-- 
2.26.2

[PATCH v3 5/8] spl: fit: Only look up FIT configuration node once

[Alexandru Gagniuc]

The configuration node a sub node under "/configurations", which
describes the components to load from "/images". We only need to
locate this node once.

However, for each component, spl_fit_get_image_name() would parse the
FIT image, looking for the correct node. Such work duplication is not
necessary. Instead, once the node is found, cache it, and re-use it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/spl/spl_fit.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index adcd6e3fcd..ed5e8b83a1 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -30,6 +30,7 @@ struct spl_fit_info {
 	const void *fit;	/* Pointer to a valid FIT blob */
 	size_t ext_data_offset;	/* Offset to FIT external data (end of FIT) */
 	int images_node;	/* FDT offset to "/images" node */
+	int conf_node;		/* FDT offset to selected configuration node */
 };
 
 __weak void board_spl_fit_post_load(const void *fit)
@@ -83,15 +84,10 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
 	struct udevice *sysinfo;
 	const char *name, *str;
 	__maybe_unused int node;
-	int conf_node;
 	int len, i;
 	bool found = true;
 
-	conf_node = fit_find_config_node(ctx->fit);
-	if (conf_node < 0)
-		return conf_node;
-
-	name = fdt_getprop(ctx->fit, conf_node, type, &len);
+	name = fdt_getprop(ctx->fit, ctx->conf_node, type, &len);
 	if (!name) {
 		debug("cannot find property '%s': %d\n", type, len);
 		return -EINVAL;
@@ -550,12 +546,15 @@ static int spl_simple_fit_read(struct spl_fit_info *ctx,
 
 static int spl_simple_fit_parse(struct spl_fit_info *ctx)
 {
-	if (IS_ENABLED(CONFIG_SPL_FIT_SIGNATURE)) {
-		int conf_offset = fit_find_config_node(ctx->fit);
+	/* Find the correct subnode under "/configurations" */
+	ctx->conf_node = fit_find_config_node(ctx->fit);
+	if (ctx->conf_node < 0)
+		return -EINVAL;
 
+	if (IS_ENABLED(CONFIG_SPL_FIT_SIGNATURE)) {
 		printf("## Checking hash(es) for config %s ... ",
-		       fit_get_name(ctx->fit, conf_offset, NULL));
-		if (fit_config_verify(ctx->fit, conf_offset))
+		       fit_get_name(ctx->fit, ctx->conf_node, NULL));
+		if (fit_config_verify(ctx->fit, ctx->conf_node))
 			return -EPERM;
 		puts("OK\n");
 	}
-- 
2.26.2

[PATCH v3 6/8] image: Do not #if guard board_fit_config_name_match() prototype

[Alexandru Gagniuc]

There's no point in guarding function prototypes with #ifdefs. If a
function is not defined, the linker will notice. Having the prototype
does not affect code size.

What the #if guard takes away is the ability to use IS_ENABLED:

	if (CONFIG_IS ENABLED(FIT_IMAGE_POST_PROCESS))
		board_fit_config_name_match(...)

When the prototype is guarded, the above form cannot be used. This
leads to the proliferation of #ifdefs, and unreadable code. The
opportunity cost of the #if guard outweighs any benefits. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 include/image.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/image.h b/include/image.h
index 00bc03bebe..fd8bd43515 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1536,8 +1536,6 @@ bool android_image_print_dtb_contents(ulong hdr_addr);
  */
 int board_fit_config_name_match(const char *name);
 
-#if defined(CONFIG_SPL_FIT_IMAGE_POST_PROCESS) || \
-	defined(CONFIG_FIT_IMAGE_POST_PROCESS)
 /**
  * board_fit_image_post_process() - Do any post-process on FIT binary data
  *
@@ -1552,7 +1550,6 @@ int board_fit_config_name_match(const char *name);
  * @return no return value (failure should be handled internally)
  */
 void board_fit_image_post_process(void **p_image, size_t *p_size);
-#endif /* CONFIG_SPL_FIT_IMAGE_POST_PROCESS */
 
 #define FDT_ERROR	((ulong)(-1))
 
-- 
2.26.2

[PATCH v3 7/8] spl: fit: Replace #ifdef blocks with more readable constructs

[Alexandru Gagniuc]

Use the IS_ENABLED() macro to control code flow, instead of the
caveman approach of sprinkling #ifdefs. Code size is not affected, as
the linker garbage-collects unused functions. However, readability is
improved significantly.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/spl/spl_fit.c | 53 ++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index ed5e8b83a1..f7782ef1a9 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -297,18 +297,16 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 		src = (void *)data;
 	}
 
-#ifdef CONFIG_SPL_FIT_SIGNATURE
-	printf("## Checking hash(es) for Image %s ... ",
-	       fit_get_name(fit, node, NULL));
-	if (!fit_image_verify_with_data(fit, node,
-					 src, length))
-		return -EPERM;
-	puts("OK\n");
-#endif
+	if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) {
+		printf("## Checking hash(es) for Image %s ... ",
+		       fit_get_name(fit, node, NULL));
+		if (!fit_image_verify_with_data(fit, node, src, length))
+			return -EPERM;
+		puts("OK\n");
+	}
 
-#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
-	board_fit_image_post_process(&src, &length);
-#endif
+	if (CONFIG_IS_ENABLED(FIT_IMAGE_POST_PROCESS))
+		board_fit_image_post_process(&src, &length);
 
 	if (IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == IH_COMP_GZIP) {
 		size = length;
@@ -373,7 +371,9 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 
 	/* Make the load-address of the FDT available for the SPL framework */
 	spl_image->fdt_addr = (void *)image_info.load_addr;
-#if !CONFIG_IS_ENABLED(FIT_IMAGE_TINY)
+	if (CONFIG_IS_ENABLED(FIT_IMAGE_TINY))
+		return 0;
+
 	if (CONFIG_IS_ENABLED(LOAD_FIT_APPLY_OVERLAY)) {
 		void *tmpbuffer = NULL;
 
@@ -431,7 +431,6 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 	ret = fdt_shrink_to_minimum(spl_image->fdt_addr, 8192);
 	if (ret < 0)
 		return ret;
-#endif
 
 	return ret;
 }
@@ -440,10 +439,12 @@ static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index,
 				   void *blob, struct spl_image_info *image)
 {
 	int ret = 0;
-#if !CONFIG_IS_ENABLED(FIT_IMAGE_TINY)
 	const char *name;
 	int node;
 
+	if (CONFIG_IS_ENABLED(FIT_IMAGE_TINY))
+		return 0;
+
 	ret = spl_fit_get_image_name(ctx, "loadables", index, &name);
 	if (ret < 0)
 		return ret;
@@ -454,15 +455,15 @@ static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index,
 				  image->size, image->entry_point,
 				  fdt_getprop(ctx->fit, node, "type", NULL),
 				  fdt_getprop(ctx->fit, node, "os", NULL));
-#endif
 	return ret;
 }
 
 static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os)
 {
-#if CONFIG_IS_ENABLED(FIT_IMAGE_TINY) && !defined(CONFIG_SPL_OS_BOOT)
-	const char *name = fdt_getprop(fit, noffset, FIT_OS_PROP, NULL);
+	if (!CONFIG_IS_ENABLED(FIT_IMAGE_TINY) || CONFIG_IS_ENABLED(OS_BOOT))
+		return fit_image_get_os(fit, noffset, os);
 
+	const char *name = fdt_getprop(fit, noffset, FIT_OS_PROP, NULL);
 	if (!name)
 		return -ENOENT;
 
@@ -477,9 +478,6 @@ static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os)
 		*os = IH_OS_INVALID;
 
 	return 0;
-#else
-	return fit_image_get_os(fit, noffset, os);
-#endif
 }
 
 /*
@@ -626,10 +624,10 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 */
 	if (node < 0)
 		node = spl_fit_get_image_node(&ctx, FIT_FIRMWARE_PROP, 0);
-#ifdef CONFIG_SPL_OS_BOOT
-	if (node < 0)
+
+	if (node < 0 && IS_ENABLED(CONFIG_SPL_OS_BOOT))
 		node = spl_fit_get_image_node(&ctx, FIT_KERNEL_PROP, 0);
-#endif
+
 	if (node < 0) {
 		debug("could not find firmware image, trying loadables...\n");
 		node = spl_fit_get_image_node(&ctx, "loadables", 0);
@@ -656,10 +654,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 */
 	if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os))
 		debug("Image OS is %s\n", genimg_get_os_name(spl_image->os));
-#if !defined(CONFIG_SPL_OS_BOOT)
-	else
+	else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT))
 		spl_image->os = IH_OS_U_BOOT;
-#endif
 
 	/*
 	 * Booting a next-stage U-Boot may require us to append the FDT.
@@ -725,9 +721,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 
 	spl_image->flags |= SPL_FIT_FOUND;
 
-#ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load(ctx.fit);
-#endif
+	if (IS_ENABLED(CONFIG_IMX_HAB))
+		board_spl_fit_post_load(ctx.fit);
 
 	return 0;
 }
-- 
2.26.2

[PATCH v3 8/8] spl: fit: Load devicetree when a Linux payload is found

[Alexandru Gagniuc]

When a FIT config specifies a devicetree, we should load it, no
questions asked. In the case of the "simple" FIT loading path, a
difficulty arises in selecting the load address of the FDT.

The default FDT location is right after the "kernel" or "firmware"
image. However, if that is an OP-TEE image, then the FDT may end up in
secure DRAM, and not be accessible to normal world kernels.

Although the best solution is to be more careful about the FDT
address, a viable workaround is to only append the FDT after a u-boot
or Linux image. This is identical to the previous logic, except that
FDT loading is extended to IH_OS_LINUX images.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl_fit.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index f7782ef1a9..5d01e5460a 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -335,6 +335,18 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 	return 0;
 }
 
+static bool os_takes_devicetree(uint8_t os)
+{
+	switch (os) {
+	case IH_OS_U_BOOT:
+		return true;
+	case IH_OS_LINUX:
+		return IS_ENABLED(CONFIG_SPL_OS_BOOT);
+	default:
+		return false;
+	}
+}
+
 static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 			      struct spl_load_info *info, ulong sector,
 			      const struct spl_fit_info *ctx)
@@ -661,9 +673,9 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * Booting a next-stage U-Boot may require us to append the FDT.
 	 * We allow this to fail, as the U-Boot image might embed its FDT.
 	 */
-	if (spl_image->os == IH_OS_U_BOOT) {
+	if (os_takes_devicetree(spl_image->os)) {
 		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
-		if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0)
+		if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
 			return ret;
 	}
 
@@ -691,7 +703,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 		if (!spl_fit_image_get_os(ctx.fit, node, &os_type))
 			debug("Loadable is %s\n", genimg_get_os_name(os_type));
 
-		if (os_type == IH_OS_U_BOOT) {
+		if (os_takes_devicetree(os_type)) {
 			spl_fit_append_fdt(&image_info, info, sector, &ctx);
 			spl_image->fdt_addr = image_info.fdt_addr;
 		}
-- 
2.26.2

[PATCH v2 1/8] spl: fit: Drop 'length' argument to board_spl_fit_post_load()

[Alexandru Gagniuc]

The size is derived from the FIT image itself. Any alignment
requirements are machine-specific and known by the board code. Thus
the total length can be derived from the FIT image and knowledge of
the platform. The 'length' argument is redundant. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/mach-imx/spl.c | 5 +++--
 common/spl/spl_fit.c    | 4 ++--
 include/spl.h           | 4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
index aa2686bb92..11255798d3 100644
--- a/arch/arm/mach-imx/spl.c
+++ b/arch/arm/mach-imx/spl.c
@@ -18,6 +18,7 @@
 #include <asm/mach-imx/hab.h>
 #include <asm/mach-imx/boot_mode.h>
 #include <g_dnl.h>
+#include <linux/libfdt.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -318,9 +319,9 @@ ulong board_spl_fit_size_align(ulong size)
 	return size;
 }
 
-void board_spl_fit_post_load(ulong load_addr, size_t length)
+void board_spl_fit_post_load(const void *fit)
 {
-	u32 offset = length - CONFIG_CSF_SIZE;
+	u32 offset = ALIGN(fdt_totalsize(fit), 0x1000);
 
 	if (imx_hab_authenticate_image(load_addr,
 				       offset + IVT_SIZE + CSF_PAD_SIZE,
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 795e2922ce..1b4a7f6b15 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -26,7 +26,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
 #endif
 
-__weak void board_spl_fit_post_load(ulong load_addr, size_t length)
+__weak void board_spl_fit_post_load(const void *fit)
 {
 }
 
@@ -722,7 +722,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	spl_image->flags |= SPL_FIT_FOUND;
 
 #ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load((ulong)fit, size);
+	board_spl_fit_post_load(fit);
 #endif
 
 	return 0;
diff --git a/include/spl.h b/include/spl.h
index 374a295fa3..cb30ef0827 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -630,9 +630,9 @@ int board_return_to_bootrom(struct spl_image_info *spl_image,
 
 /**
  * board_spl_fit_post_load - allow process images after loading finished
- *
+ * @fit: Pointer to a valid Flattened Image Tree blob
  */
-void board_spl_fit_post_load(ulong load_addr, size_t length);
+void board_spl_fit_post_load(const void *fit);
 
 /**
  * board_spl_fit_size_align - specific size align before processing payload
-- 
2.26.2

[PATCH v2 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Alexandru Gagniuc]

The logical steps in spl_load_simple_fit() are difficult to follow.
I think the long comments, ifdefs, and ungodly number of variables
seriously affect the readability. In particular, it violates section 6
of the coding style, paragraphs (3), and (4).

The purpose of this patch is to improve the situation by
  - Factoring out initialization and parsing to separate functions
  - Reduce the number of variables by using a context structure
This change introduces no functional changes.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl_fit.c | 90 +++++++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 30 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 1b4a7f6b15..bcc943b0b8 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -26,6 +26,12 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_SYS_BOOTM_LEN	(64 << 20)
 #endif
 
+struct spl_fit_info {
+	const void *fit;	/* Pointer to a valid FIT blob */
+	size_t ext_data_offset;	/* Offset to FIT external data (end of FIT) */
+	int images_node;	/* FDT offset to "/images" node */
+};
+
 __weak void board_spl_fit_post_load(const void *fit)
 {
 }
@@ -521,28 +527,22 @@ __weak bool spl_load_simple_fit_skip_processing(void)
 	return false;
 }
 
-int spl_load_simple_fit(struct spl_image_info *spl_image,
-			struct spl_load_info *info, ulong sector, void *fit)
+static int spl_simple_fit_read(struct spl_fit_info *ctx,
+			       struct spl_load_info *info, ulong sector,
+			       const void *fit_header)
 {
+	unsigned long count, size;
 	int sectors;
-	ulong size, hsize;
-	unsigned long count;
-	struct spl_image_info image_info;
-	int node = -1;
-	int images, ret;
-	int base_offset;
-	int index = 0;
-	int firmware_node;
+	void *buf;
 
 	/*
 	 * For FIT with external data, figure out where the external images
 	 * start. This is the base for the data-offset properties in each
 	 * image.
 	 */
-	size = fdt_totalsize(fit);
-	size = (size + 3) & ~3;
+	size = ALIGN(fdt_totalsize(fit_header), 4);
 	size = board_spl_fit_size_align(size);
-	base_offset = (size + 3) & ~3;
+	ctx->ext_data_offset = ALIGN(size, 4);
 
 	/*
 	 * So far we only have one block of data from the FIT. Read the entire
@@ -552,36 +552,66 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * For FIT with external data, data is not loaded in this step.
 	 */
 	sectors = get_aligned_image_size(info, size, 0);
-	hsize = sectors * info->bl_len;
-	fit = spl_get_fit_load_buffer(hsize);
-	count = info->read(info, sector, sectors, fit);
-	debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu, size=0x%lx\n",
-	      sector, sectors, fit, count, size);
+	buf = spl_get_fit_load_buffer(sectors * info->bl_len);
 
-	if (count == 0)
-		return -EIO;
+	count = info->read(info, sector, sectors, buf);
+	ctx->fit = buf;
+	debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu, size=0x%lx\n",
+	      sector, sectors, buf, count, size);
 
-	/* skip further processing if requested to enable load-only use cases */
-	if (spl_load_simple_fit_skip_processing())
-		return 0;
+	return (count == 0) ? -EIO : 0;
+}
 
+static int spl_simple_fit_parse(struct spl_fit_info *ctx)
+{
 	if (IS_ENABLED(CONFIG_SPL_FIT_SIGNATURE)) {
-		int conf_offset = fit_find_config_node(fit);
+		int conf_offset = fit_find_config_node(ctx->fit);
 
 		printf("## Checking hash(es) for config %s ... ",
-		       fit_get_name(fit, conf_offset, NULL));
-		if (fit_config_verify(fit, conf_offset))
+		       fit_get_name(ctx->fit, conf_offset, NULL));
+		if (fit_config_verify(ctx->fit, conf_offset))
 			return -EPERM;
 		puts("OK\n");
 	}
 
 	/* find the node holding the images information */
-	images = fdt_path_offset(fit, FIT_IMAGES_PATH);
-	if (images < 0) {
-		debug("%s: Cannot find /images node: %d\n", __func__, images);
-		return -1;
+	ctx->images_node = fdt_path_offset(ctx->fit, FIT_IMAGES_PATH);
+	if (ctx->images_node < 0) {
+		debug("%s: Cannot find /images node: %d\n", __func__,
+		      ctx->images_node);
+		return -EINVAL;
 	}
 
+	return 0;
+}
+
+int spl_load_simple_fit(struct spl_image_info *spl_image,
+			struct spl_load_info *info, ulong sector, void *fit)
+{
+	struct spl_image_info image_info;
+	struct spl_fit_info ctx;
+	int node = -1;
+	int images, ret;
+	int base_offset;
+	int index = 0;
+	int firmware_node;
+
+	ret = spl_simple_fit_read(&ctx, info, sector, fit);
+	if (ret < 0)
+		return ret;
+
+	/* skip further processing if requested to enable load-only use cases */
+	if (spl_load_simple_fit_skip_processing())
+		return 0;
+
+	ret = spl_simple_fit_parse(&ctx);
+	if (ret < 0)
+		return ret;
+
+	images = ctx.images_node;
+	fit = (void *)ctx.fit;
+	base_offset = ctx.ext_data_offset;
+
 #ifdef CONFIG_SPL_FPGA
 	node = spl_fit_get_image_node(fit, images, "fpga", 0);
 	if (node >= 0) {
-- 
2.26.2

[PATCH v2 2/8] spl: fit: Factor out FIT parsing and use a context struct

[Simon Glass]

On Tue, 22 Dec 2020 at 16:54, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> The logical steps in spl_load_simple_fit() are difficult to follow.
> I think the long comments, ifdefs, and ungodly number of variables
> seriously affect the readability. In particular, it violates section 6
> of the coding style, paragraphs (3), and (4).
>
> The purpose of this patch is to improve the situation by
>   - Factoring out initialization and parsing to separate functions
>   - Reduce the number of variables by using a context structure
> This change introduces no functional changes.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/spl/spl_fit.c | 90 +++++++++++++++++++++++++++++---------------
>  1 file changed, 60 insertions(+), 30 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 3/8] spl: fit: Pass FIT context via a structure pointer

[Alexandru Gagniuc]

Several loose arguments describe the FIT image. They are thus related,
and it makes sense to pass them together, in a structure. Examples
include the FIT blob pointer, offset to FDT nodes, and the offset to
external data.

Use a spl_fit_info structure to group these parameters.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/spl/spl_fit.c | 101 ++++++++++++++++++-------------------------
 1 file changed, 43 insertions(+), 58 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index bcc943b0b8..5a06e8a9db 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -76,7 +76,7 @@ static int find_node_from_desc(const void *fit, int node, const char *str)
  *
  * Return:	0 on success, or a negative error number
  */
-static int spl_fit_get_image_name(const void *fit, int images,
+static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
 				  const char *type, int index,
 				  const char **outname)
 {
@@ -87,21 +87,21 @@ static int spl_fit_get_image_name(const void *fit, int images,
 	int len, i;
 	bool found = true;
 
-	conf_node = fit_find_config_node(fit);
+	conf_node = fit_find_config_node(ctx->fit);
 	if (conf_node < 0) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
 		printf("No matching DT out of these options:\n");
-		for (node = fdt_first_subnode(fit, conf_node);
+		for (node = fdt_first_subnode(ctx->fit, conf_node);
 		     node >= 0;
-		     node = fdt_next_subnode(fit, node)) {
-			name = fdt_getprop(fit, node, "description", &len);
+		     node = fdt_next_subnode(ctx->fit, node)) {
+			name = fdt_getprop(ctx->fit, node, "description", &len);
 			printf("   %s\n", name);
 		}
 #endif
 		return conf_node;
 	}
 
-	name = fdt_getprop(fit, conf_node, type, &len);
+	name = fdt_getprop(ctx->fit, conf_node, type, &len);
 	if (!name) {
 		debug("cannot find property '%s': %d\n", type, len);
 		return -EINVAL;
@@ -135,11 +135,11 @@ static int spl_fit_get_image_name(const void *fit, int images,
 			 * node name.
 			 */
 			int node;
-			int images = fdt_path_offset(fit, FIT_IMAGES_PATH);
+			int images = fdt_path_offset(ctx->fit, FIT_IMAGES_PATH);
 
-			node = find_node_from_desc(fit, images, str);
+			node = find_node_from_desc(ctx->fit, images, str);
 			if (node > 0)
-				str = fdt_get_name(fit, node, NULL);
+				str = fdt_get_name(ctx->fit, node, NULL);
 
 			found = true;
 		}
@@ -166,20 +166,20 @@ static int spl_fit_get_image_name(const void *fit, int images,
  * Return:	the node offset of the respective image node or a negative
  *		error number.
  */
-static int spl_fit_get_image_node(const void *fit, int images,
+static int spl_fit_get_image_node(const struct spl_fit_info *ctx,
 				  const char *type, int index)
 {
 	const char *str;
 	int err;
 	int node;
 
-	err = spl_fit_get_image_name(fit, images, type, index, &str);
+	err = spl_fit_get_image_name(ctx, type, index, &str);
 	if (err)
 		return err;
 
 	debug("%s: '%s'\n", type, str);
 
-	node = fdt_subnode_offset(fit, images, str);
+	node = fdt_subnode_offset(ctx->fit, ctx->images_node, str);
 	if (node < 0) {
 		pr_err("cannot find image node '%s': %d\n", str, node);
 		return -EINVAL;
@@ -230,10 +230,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
  * spl_load_fit_image(): load the image described in a certain FIT node
  * @info:	points to information about the device to load data from
  * @sector:	the start sector of the FIT image on the device
- * @fit:	points to the flattened device tree blob describing the FIT
- *		image
- * @base_offset: the beginning of the data area containing the actual
- *		image data, relative to the beginning of the FIT
+ * @ctx:	points to the FIT context structure
  * @node:	offset of the DT node describing the image to load (relative
  *		to @fit)
  * @image_info:	will be filled with information about the loaded image
@@ -244,7 +241,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
  * Return:	0 on success or a negative error number.
  */
 static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
-			      void *fit, ulong base_offset, int node,
+			      const struct spl_fit_info *ctx, int node,
 			      struct spl_image_info *image_info)
 {
 	int offset;
@@ -258,6 +255,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 	int align_len = ARCH_DMA_MINALIGN - 1;
 	uint8_t image_comp = -1, type = -1;
 	const void *data;
+	const void *fit = ctx->fit;
 	bool external_data = false;
 
 	if (IS_ENABLED(CONFIG_SPL_FPGA) ||
@@ -279,7 +277,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 	if (!fit_image_get_data_position(fit, node, &offset)) {
 		external_data = true;
 	} else if (!fit_image_get_data_offset(fit, node, &offset)) {
-		offset += base_offset;
+		offset += ctx->ext_data_offset;
 		external_data = true;
 	}
 
@@ -355,7 +353,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 
 static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 			      struct spl_load_info *info, ulong sector,
-			      void *fit, int images, ulong base_offset)
+			      const struct spl_fit_info *ctx)
 {
 	struct spl_image_info image_info;
 	int node, ret = 0, index = 0;
@@ -367,7 +365,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 	image_info.load_addr = spl_image->load_addr + spl_image->size;
 
 	/* Figure out which device tree the board wants to use */
-	node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, index++);
+	node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index++);
 	if (node < 0) {
 		debug("%s: cannot find FDT node\n", __func__);
 
@@ -381,7 +379,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 		else
 			return node;
 	} else {
-		ret = spl_load_fit_image(info, sector, fit, base_offset, node,
+		ret = spl_load_fit_image(info, sector, ctx, node,
 					 &image_info);
 		if (ret < 0)
 			return ret;
@@ -394,8 +392,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 		void *tmpbuffer = NULL;
 
 		for (; ; index++) {
-			node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP,
-						      index);
+			node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index);
 			if (node == -E2BIG) {
 				debug("%s: No additional FDT node\n", __func__);
 				break;
@@ -418,7 +415,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 					      __func__);
 			}
 			image_info.load_addr = (ulong)tmpbuffer;
-			ret = spl_load_fit_image(info, sector, fit, base_offset,
+			ret = spl_load_fit_image(info, sector, ctx,
 						 node, &image_info);
 			if (ret < 0)
 				break;
@@ -433,12 +430,12 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 							(void *)image_info.load_addr);
 			if (ret) {
 				pr_err("failed to apply DT overlay %s\n",
-				       fit_get_name(fit, node, NULL));
+				       fit_get_name(ctx->fit, node, NULL));
 				break;
 			}
 
 			debug("%s: DT overlay %s applied\n", __func__,
-			      fit_get_name(fit, node, NULL));
+			      fit_get_name(ctx->fit, node, NULL));
 		}
 		free(tmpbuffer);
 		if (ret)
@@ -453,7 +450,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 	return ret;
 }
 
-static int spl_fit_record_loadable(const void *fit, int images, int index,
+static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index,
 				   void *blob, struct spl_image_info *image)
 {
 	int ret = 0;
@@ -461,17 +458,16 @@ static int spl_fit_record_loadable(const void *fit, int images, int index,
 	const char *name;
 	int node;
 
-	ret = spl_fit_get_image_name(fit, images, "loadables",
-				     index, &name);
+	ret = spl_fit_get_image_name(ctx, "loadables", index, &name);
 	if (ret < 0)
 		return ret;
 
-	node = spl_fit_get_image_node(fit, images, "loadables", index);
+	node = spl_fit_get_image_node(ctx, "loadables", index);
 
 	ret = fdt_record_loadable(blob, index, name, image->load_addr,
 				  image->size, image->entry_point,
-				  fdt_getprop(fit, node, "type", NULL),
-				  fdt_getprop(fit, node, "os", NULL));
+				  fdt_getprop(ctx->fit, node, "type", NULL),
+				  fdt_getprop(ctx->fit, node, "os", NULL));
 #endif
 	return ret;
 }
@@ -591,8 +587,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	struct spl_image_info image_info;
 	struct spl_fit_info ctx;
 	int node = -1;
-	int images, ret;
-	int base_offset;
+	int ret;
 	int index = 0;
 	int firmware_node;
 
@@ -608,16 +603,11 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	if (ret < 0)
 		return ret;
 
-	images = ctx.images_node;
-	fit = (void *)ctx.fit;
-	base_offset = ctx.ext_data_offset;
-
 #ifdef CONFIG_SPL_FPGA
-	node = spl_fit_get_image_node(fit, images, "fpga", 0);
+	node = spl_fit_get_image_node(&ctx, "fpga", 0);
 	if (node >= 0) {
 		/* Load the image and set up the spl_image structure */
-		ret = spl_load_fit_image(info, sector, fit, base_offset, node,
-					 spl_image);
+		ret = spl_load_fit_image(info, sector, ctx, node, spl_image);
 		if (ret) {
 			printf("%s: Cannot load the FPGA: %i\n", __func__, ret);
 			return ret;
@@ -646,15 +636,14 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 *   - fall back to using the first 'loadables' entry
 	 */
 	if (node < 0)
-		node = spl_fit_get_image_node(fit, images, FIT_FIRMWARE_PROP,
-					      0);
+		node = spl_fit_get_image_node(&ctx, FIT_FIRMWARE_PROP, 0);
 #ifdef CONFIG_SPL_OS_BOOT
 	if (node < 0)
-		node = spl_fit_get_image_node(fit, images, FIT_KERNEL_PROP, 0);
+		node = spl_fit_get_image_node(&ctx, FIT_KERNEL_PROP, 0);
 #endif
 	if (node < 0) {
 		debug("could not find firmware image, trying loadables...\n");
-		node = spl_fit_get_image_node(fit, images, "loadables", 0);
+		node = spl_fit_get_image_node(&ctx, "loadables", 0);
 		/*
 		 * If we pick the U-Boot image from "loadables", start at
 		 * the second image when later loading additional images.
@@ -668,8 +657,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	}
 
 	/* Load the image and set up the spl_image structure */
-	ret = spl_load_fit_image(info, sector, fit, base_offset, node,
-				 spl_image);
+	ret = spl_load_fit_image(info, sector, &ctx, node, spl_image);
 	if (ret)
 		return ret;
 
@@ -677,7 +665,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * For backward compatibility, we treat the first node that is
 	 * as a U-Boot image, if no OS-type has been declared.
 	 */
-	if (!spl_fit_image_get_os(fit, node, &spl_image->os))
+	if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os))
 		debug("Image OS is %s\n", genimg_get_os_name(spl_image->os));
 #if !defined(CONFIG_SPL_OS_BOOT)
 	else
@@ -689,8 +677,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * We allow this to fail, as the U-Boot image might embed its FDT.
 	 */
 	if (spl_image->os == IH_OS_U_BOOT) {
-		ret = spl_fit_append_fdt(spl_image, info, sector, fit,
-					 images, base_offset);
+		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
 		if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0)
 			return ret;
 	}
@@ -700,7 +687,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	for (; ; index++) {
 		uint8_t os_type = IH_OS_INVALID;
 
-		node = spl_fit_get_image_node(fit, images, "loadables", index);
+		node = spl_fit_get_image_node(&ctx, "loadables", index);
 		if (node < 0)
 			break;
 
@@ -712,17 +699,15 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 		if (firmware_node == node)
 			continue;
 
-		ret = spl_load_fit_image(info, sector, fit, base_offset, node,
-					 &image_info);
+		ret = spl_load_fit_image(info, sector, &ctx, node, &image_info);
 		if (ret < 0)
 			continue;
 
-		if (!spl_fit_image_get_os(fit, node, &os_type))
+		if (!spl_fit_image_get_os(ctx.fit, node, &os_type))
 			debug("Loadable is %s\n", genimg_get_os_name(os_type));
 
 		if (os_type == IH_OS_U_BOOT) {
-			spl_fit_append_fdt(&image_info, info, sector,
-					   fit, images, base_offset);
+			spl_fit_append_fdt(&image_info, info, sector, &ctx);
 			spl_image->fdt_addr = image_info.fdt_addr;
 		}
 
@@ -736,7 +721,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 
 		/* Record our loadables into the FDT */
 		if (spl_image->fdt_addr)
-			spl_fit_record_loadable(fit, images, index,
+			spl_fit_record_loadable(&ctx, index,
 						spl_image->fdt_addr,
 						&image_info);
 	}
@@ -752,7 +737,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	spl_image->flags |= SPL_FIT_FOUND;
 
 #ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load(fit);
+	board_spl_fit_post_load(ctx.fit);
 #endif
 
 	return 0;
-- 
2.26.2

[PATCH v2 4/8] spl: fit: Remove useless loop in spl_fit_get_image_name()

[Alexandru Gagniuc]

When a desired configuration is not found, conf_node will have a
negative value. Thus the for loop will start at the root "/" node of
the image, print the "/description" property, and stop.

It appears the intent of the loop was to print the names of the
subnodes under "/configurations". We would need the offset to the
"/configurations" node, which is abstracted by fit_find_config_node().

This change agrees that abstracting the node offset is the correct
design, and we shouldn't be parsing the configurations manually. Thus
the loop in spl_fit_get_image_name() is useless. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/spl/spl_fit.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 5a06e8a9db..adcd6e3fcd 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -88,18 +88,8 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
 	bool found = true;
 
 	conf_node = fit_find_config_node(ctx->fit);
-	if (conf_node < 0) {
-#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-		printf("No matching DT out of these options:\n");
-		for (node = fdt_first_subnode(ctx->fit, conf_node);
-		     node >= 0;
-		     node = fdt_next_subnode(ctx->fit, node)) {
-			name = fdt_getprop(ctx->fit, node, "description", &len);
-			printf("   %s\n", name);
-		}
-#endif
+	if (conf_node < 0)
 		return conf_node;
-	}
 
 	name = fdt_getprop(ctx->fit, conf_node, type, &len);
 	if (!name) {
-- 
2.26.2

[PATCH v2 5/8] spl: fit: Only look up FIT configuration node once

[Alexandru Gagniuc]

The configuration node a sub node under "/configurations", which
describes the components to load from "/images". We only need to
locate this node once.

However, for each component, spl_fit_get_image_name() would parse the
FIT image, looking for the correct node. Such work duplication is not
necessary. Instead, once the node is found, cache it, and re-use it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/spl/spl_fit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index adcd6e3fcd..c3da5b1443 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -30,6 +30,7 @@ struct spl_fit_info {
 	const void *fit;	/* Pointer to a valid FIT blob */
 	size_t ext_data_offset;	/* Offset to FIT external data (end of FIT) */
 	int images_node;	/* FDT offset to "/images" node */
+	int conf_node;		/* FDT offset to selected configuration node */
 };
 
 __weak void board_spl_fit_post_load(const void *fit)
-- 
2.26.2

[PATCH v2 6/8] image: Do not #if guard board_fit_config_name_match() prototype

[Alexandru Gagniuc]

There's no point in guarding function prototypes with #ifdefs. If a
function is not defined, the linker will notice. Having the prototype
does not affect code size.

What the #if guard takes away is the ability to use IS_ENABLED:

	if (CONFIG_IS ENABLED(FIT_IMAGE_POST_PROCESS))
		board_fit_config_name_match(...)

When the prototype is guarded, the above form cannot be used. This
leads to the proliferation of #ifdefs, and unreadable code. The
opportunity cost of the #if guard outweighs any benefits. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 include/image.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/image.h b/include/image.h
index 00bc03bebe..fd8bd43515 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1536,8 +1536,6 @@ bool android_image_print_dtb_contents(ulong hdr_addr);
  */
 int board_fit_config_name_match(const char *name);
 
-#if defined(CONFIG_SPL_FIT_IMAGE_POST_PROCESS) || \
-	defined(CONFIG_FIT_IMAGE_POST_PROCESS)
 /**
  * board_fit_image_post_process() - Do any post-process on FIT binary data
  *
@@ -1552,7 +1550,6 @@ int board_fit_config_name_match(const char *name);
  * @return no return value (failure should be handled internally)
  */
 void board_fit_image_post_process(void **p_image, size_t *p_size);
-#endif /* CONFIG_SPL_FIT_IMAGE_POST_PROCESS */
 
 #define FDT_ERROR	((ulong)(-1))
 
-- 
2.26.2

[PATCH v2 7/8] spl: fit: Replace #ifdef blocks with more readable constructs

[Alexandru Gagniuc]

Use the IS_ENABLED() macro to control code flow, instead of the
caveman approach of sprinkling #ifdefs. Code size is not affected, as
the linker garbage-collects unused functions. However, readability is
improved significantly.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/spl/spl_fit.c | 53 ++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index c3da5b1443..35d7d8ec40 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -302,18 +302,16 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 		src = (void *)data;
 	}
 
-#ifdef CONFIG_SPL_FIT_SIGNATURE
-	printf("## Checking hash(es) for Image %s ... ",
-	       fit_get_name(fit, node, NULL));
-	if (!fit_image_verify_with_data(fit, node,
-					 src, length))
-		return -EPERM;
-	puts("OK\n");
-#endif
+	if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) {
+		printf("## Checking hash(es) for Image %s ... ",
+		       fit_get_name(fit, node, NULL));
+		if (!fit_image_verify_with_data(fit, node, src, length))
+			return -EPERM;
+		puts("OK\n");
+	}
 
-#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
-	board_fit_image_post_process(&src, &length);
-#endif
+	if (CONFIG_IS_ENABLED(FIT_IMAGE_POST_PROCESS))
+		board_fit_image_post_process(&src, &length);
 
 	if (IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == IH_COMP_GZIP) {
 		size = length;
@@ -378,7 +376,9 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 
 	/* Make the load-address of the FDT available for the SPL framework */
 	spl_image->fdt_addr = (void *)image_info.load_addr;
-#if !CONFIG_IS_ENABLED(FIT_IMAGE_TINY)
+	if (CONFIG_IS_ENABLED(FIT_IMAGE_TINY))
+		return 0;
+
 	if (CONFIG_IS_ENABLED(LOAD_FIT_APPLY_OVERLAY)) {
 		void *tmpbuffer = NULL;
 
@@ -436,7 +436,6 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 	ret = fdt_shrink_to_minimum(spl_image->fdt_addr, 8192);
 	if (ret < 0)
 		return ret;
-#endif
 
 	return ret;
 }
@@ -445,10 +444,12 @@ static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index,
 				   void *blob, struct spl_image_info *image)
 {
 	int ret = 0;
-#if !CONFIG_IS_ENABLED(FIT_IMAGE_TINY)
 	const char *name;
 	int node;
 
+	if (CONFIG_IS_ENABLED(FIT_IMAGE_TINY))
+		return 0;
+
 	ret = spl_fit_get_image_name(ctx, "loadables", index, &name);
 	if (ret < 0)
 		return ret;
@@ -459,15 +460,15 @@ static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index,
 				  image->size, image->entry_point,
 				  fdt_getprop(ctx->fit, node, "type", NULL),
 				  fdt_getprop(ctx->fit, node, "os", NULL));
-#endif
 	return ret;
 }
 
 static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os)
 {
-#if CONFIG_IS_ENABLED(FIT_IMAGE_TINY) && !defined(CONFIG_SPL_OS_BOOT)
-	const char *name = fdt_getprop(fit, noffset, FIT_OS_PROP, NULL);
+	if (!CONFIG_IS_ENABLED(FIT_IMAGE_TINY) || CONFIG_IS_ENABLED(OS_BOOT))
+		return fit_image_get_os(fit, noffset, os);
 
+	const char *name = fdt_getprop(fit, noffset, FIT_OS_PROP, NULL);
 	if (!name)
 		return -ENOENT;
 
@@ -482,9 +483,6 @@ static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os)
 		*os = IH_OS_INVALID;
 
 	return 0;
-#else
-	return fit_image_get_os(fit, noffset, os);
-#endif
 }
 
 /*
@@ -628,10 +626,10 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 */
 	if (node < 0)
 		node = spl_fit_get_image_node(&ctx, FIT_FIRMWARE_PROP, 0);
-#ifdef CONFIG_SPL_OS_BOOT
-	if (node < 0)
+
+	if (node < 0 && IS_ENABLED(CONFIG_SPL_OS_BOOT))
 		node = spl_fit_get_image_node(&ctx, FIT_KERNEL_PROP, 0);
-#endif
+
 	if (node < 0) {
 		debug("could not find firmware image, trying loadables...\n");
 		node = spl_fit_get_image_node(&ctx, "loadables", 0);
@@ -658,10 +656,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 */
 	if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os))
 		debug("Image OS is %s\n", genimg_get_os_name(spl_image->os));
-#if !defined(CONFIG_SPL_OS_BOOT)
-	else
+	else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT))
 		spl_image->os = IH_OS_U_BOOT;
-#endif
 
 	/*
 	 * Booting a next-stage U-Boot may require us to append the FDT.
@@ -727,9 +723,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 
 	spl_image->flags |= SPL_FIT_FOUND;
 
-#ifdef CONFIG_IMX_HAB
-	board_spl_fit_post_load(ctx.fit);
-#endif
+	if (IS_ENABLED(CONFIG_IMX_HAB))
+		board_spl_fit_post_load(ctx.fit);
 
 	return 0;
 }
-- 
2.26.2

[PATCH v2 8/8] spl: fit: Load devicetree when a Linux payload is found

[Alexandru Gagniuc]

When a FIT config specifies a devicetree, we should load it, no
questions asked. In the case of the "simple" FIT loading path, a
difficulty arises in selecting the load address of the FDT.

The default FDT location is right after the "kernel" or "firmware"
image. However, if that is an OP-TEE image, then the FDT may end up in
secure DRAM, and not be accessible to normal world kernels.

Although the best solution is to be more careful about the FDT
address, a viable workaround is to only append the FDT after a u-boot
or Linux image. This is identical to the previous logic, except that
FDT loading is extended to IH_OS_LINUX images.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl_fit.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 35d7d8ec40..7b42b41fc4 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -340,6 +340,18 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 	return 0;
 }
 
+static bool os_takes_devicetree(uint8_t os)
+{
+	switch (os) {
+	case IH_OS_U_BOOT:
+		return true;
+	case IH_OS_LINUX:
+		return IS_ENABLED(CONFIG_SPL_OS_BOOT);
+	default:
+		return false;
+	}
+}
+
 static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 			      struct spl_load_info *info, ulong sector,
 			      const struct spl_fit_info *ctx)
@@ -663,9 +675,9 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	 * Booting a next-stage U-Boot may require us to append the FDT.
 	 * We allow this to fail, as the U-Boot image might embed its FDT.
 	 */
-	if (spl_image->os == IH_OS_U_BOOT) {
+	if (os_takes_devicetree(spl_image->os)) {
 		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
-		if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0)
+		if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
 			return ret;
 	}
 
@@ -693,7 +705,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 		if (!spl_fit_image_get_os(ctx.fit, node, &os_type))
 			debug("Loadable is %s\n", genimg_get_os_name(os_type));
 
-		if (os_type == IH_OS_U_BOOT) {
+		if (os_takes_devicetree(os_type)) {
 			spl_fit_append_fdt(&image_info, info, sector, &ctx);
 			spl_image->fdt_addr = image_info.fdt_addr;
 		}
-- 
2.26.2