[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Andre Przywara]

Currently the FIT format is not used to its full potential in the SPL:
It only loads the first image from the /images node and appends the
proper FDT.
Some boards and platforms would benefit from loading more images before
starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.

This series tries to solve this in a board agnostic and generic way:
We extend the SPL FIT loading scheme to allow loading multiple images.
So apart from loading the image which is referenced by the "firmware"
property in the respective configuration node and placing the DTB right
behind it, we iterate over all strings in the "loadable" property.
Each image referenced there will be loaded to its specified load address.
The entry point U-Boot eventually branches to will be taken from the
first image to explicitly provide the "entry" property, or, if none
of them does so, from the load address of the "firmware" image.
This keeps the scheme compatible with the FIT images our Makefile creates
automatically at the moment.

Apart from the already mentioned ATF scenario this opens up more usage
scenarios, of which the commit message of patch 04/11 lists some.

The first three patches rework the SPL FIT support to be more flexible
and to allow easier usage in the fourth patch, which introduces the
multiple-image loading facility.
The remaining patches enable that support for the Pine64 board to make
its SPL support finally useful and to demonstrate usage of this scheme:
patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
compilation of the SPL with FIT support enabled. Patch 8 implements the
board selector routine, which selects either the Pine64 or Pine64+ DTB
depending on the detected DRAM size. Patch 9 enables SPL FIT support in
the Pine64 defconfig.
To demonstrate the usage, patch 10 provides a FIT source file, which
loads and executes ATF before the U-Boot proper. Users are expected to
compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
then write the resulting file behind the SPL on an SD card (or any other
U-Boot supported boot media, for that matter).
Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
which allows to load ATF on boards with SPI flash as well.

Questions:
1) Is this scheme the right one (usage of "firmware" and "loadables",
   determination of entry point)? Shall we make use of the "setup"
   property?
2) Shall we extend mkimage to allow supplying "loadable" files on the
   command line, which would allow to build the .itb file automatically?
3) Is providing the .its source file for a (family of) boards the right
   way?
4) Does this break any boards which already use SPL FIT loading?

And for the Pine64 part:
5) Is extending the usable SPL size like in patch 5-7 acceptable?

I have a more generic solution for the .dtb selection in mind: Based on
some patch from Siarhei we store the .dtb filename in the SPL header and
select the .dtb from the FIT image by simply matching the name. This would
allow _one_ build supporting multiple boards. The actual board name would
need to written into the SPL header or could be copied from there when
updating the image.
I can provide the patches once we agreed upon this series.

Please let me know what you think!

Cheers,
Andre.

Andre Przywara (11):
  SPL: FIT: refactor FDT loading
  SPL: FIT: rework U-Boot image loading
  SPL: FIT: factor out spl_load_fit_image()
  SPL: FIT: allow loading multiple images
  tools: mksunxiboot: allow larger SPL binaries
  sunxi: A64: SPL: allow large SPL binary
  sunxi: A64: move SPL stack to end of SRAM A2
  sunxi: SPL: add FIT config selector for Pine64 boards
  sunxi: Pine64: defconfig: enable SPL FIT support
  sunxi: Pine64: add FIT image source
  SPL: SPI: sunxi: add SPL FIT image support

 board/sunxi/board.c             |  13 +++
 board/sunxi/pine64_atf.its      |  54 +++++++++
 common/spl/spl_fit.c            | 241 +++++++++++++++++++++++-----------------
 configs/pine64_plus_defconfig   |   5 +
 drivers/mtd/spi/sunxi_spi_spl.c |  39 +++++--
 include/configs/sunxi-common.h  |   6 +-
 scripts/Makefile.spl            |   7 +-
 tools/mksunxiboot.c             |  51 ++++++---
 8 files changed, 290 insertions(+), 126 deletions(-)
 create mode 100644 board/sunxi/pine64_atf.its

-- 
2.8.2

[U-Boot] [RFC PATCH 01/11] SPL: FIT: refactor FDT loading

[Andre Przywara]

Currently the SPL FIT loader uses the spl_fit_select_fdt() function to
find the offset to the right DTB within the FIT image.
For this it iterates over all subnodes of the /configuration node in
the FIT tree and compares all "description" strings therein using a
board specific matching function.
If that finds a match, it uses the string in the "fdt" property of that
subnode to locate the matching subnode in the /images node, which points
to the DTB data.
Now this works very well, but is quite specific to cover this particular
use case. To open up the door for a more generic usage, let's split this
function into:
1) a function that just returns the node offset for the matching
   configuration node (spl_fit_find_config_node())
2) a function that returns the image data any given property in a given
   configuration node points to, additionally using a given index into
   a possbile list of strings (spl_fit_select_index())
This allows us to replace the specific function above by asking for the
image the _first string of the "fdt" property_ in the matching
configuration subnode points to.

This patch introduces no functional changes, it just refactors the code
to allow reusing it later.

(diff is overly clever here and produces a hard-to-read patch, so I
recommend to throw a look at the result instead).

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 common/spl/spl_fit.c | 82 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 51 insertions(+), 31 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index aae556f..c4e2f02 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -22,13 +22,11 @@ static ulong fdt_getprop_u32(const void *fdt, int node, const char *prop)
 	return fdt32_to_cpu(*cell);
 }
 
-static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp)
+static int spl_fit_find_config_node(const void *fdt)
 {
-	const char *name, *fdt_name;
-	int conf, node, fdt_node;
-	int len;
+	const char *name;
+	int conf, node, len;
 
-	*fdt_offsetp = 0;
 	conf = fdt_path_offset(fdt, FIT_CONFS_PATH);
 	if (conf < 0) {
 		debug("%s: Cannot find /configurations node: %d\n", __func__,
@@ -50,39 +48,60 @@ static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp)
 			continue;
 
 		debug("Selecting config '%s'", name);
-		fdt_name = fdt_getprop(fdt, node, FIT_FDT_PROP, &len);
-		if (!fdt_name) {
-			debug("%s: Cannot find fdt name property: %d\n",
-			      __func__, len);
-			return -EINVAL;
-		}
 
-		debug(", fdt '%s'\n", fdt_name);
-		fdt_node = fdt_subnode_offset(fdt, images, fdt_name);
-		if (fdt_node < 0) {
-			debug("%s: Cannot find fdt node '%s': %d\n",
-			      __func__, fdt_name, fdt_node);
-			return -EINVAL;
+		return node;
+	}
+
+	return -1;
+}
+
+static int spl_fit_select_index(const void *fit, int images, int *offsetp,
+				const char *type, int index)
+{
+	const char *name, *img_name;
+	int node, conf_node;
+	int len, i;
+
+	*offsetp = 0;
+	conf_node = spl_fit_find_config_node(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);
+		     node >= 0;
+		     node = fdt_next_subnode(fit, node)) {
+			name = fdt_getprop(fit, node, "description", &len);
+			printf("   %s\n", name);
 		}
+#endif
+		return -ENOENT;
+	}
 
-		*fdt_offsetp = fdt_getprop_u32(fdt, fdt_node, "data-offset");
-		len = fdt_getprop_u32(fdt, fdt_node, "data-size");
-		debug("FIT: Selected '%s'\n", name);
+	img_name = fdt_getprop(fit, conf_node, type, &len);
+	if (!img_name) {
+		debug("cannot find property '%s': %d\n", type, len);
+		return -EINVAL;
+	}
 
-		return len;
+	for (i = 0; i < index; i++) {
+		img_name = strchr(img_name, '\0') + 1;
+		if (*img_name == '\0') {
+			debug("no string for index %d\n", index);
+			return -E2BIG;
+		}
 	}
 
-#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-	printf("No matching DT out of these options:\n");
-	for (node = fdt_first_subnode(fdt, conf);
-	     node >= 0;
-	     node = fdt_next_subnode(fdt, node)) {
-		name = fdt_getprop(fdt, node, "description", &len);
-		printf("   %s\n", name);
+	debug("%s: '%s'\n", type, img_name);
+	node = fdt_subnode_offset(fit, images, img_name);
+	if (node < 0) {
+		debug("cannot find image node '%s': %d\n", img_name, node);
+		return -EINVAL;
 	}
-#endif
 
-	return -ENOENT;
+	*offsetp = fdt_getprop_u32(fit, node, "data-offset");
+	len = fdt_getprop_u32(fit, node, "data-size");
+
+	return len;
 }
 
 static int get_aligned_image_offset(struct spl_load_info *info, int offset)
@@ -218,7 +237,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	memcpy(dst, src, data_size);
 
 	/* Figure out which device tree the board wants to use */
-	fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset);
+	fdt_len = spl_fit_select_index(fit, images, &fdt_offset,
+				       FIT_FDT_PROP, 0);
 	if (fdt_len < 0)
 		return fdt_len;
 
-- 
2.8.2

[U-Boot] [RFC PATCH 01/11] SPL: FIT: refactor FDT loading

[Lokesh Vutla]

On Friday 20 January 2017 07:23 AM, Andre Przywara wrote:
> Currently the SPL FIT loader uses the spl_fit_select_fdt() function to
> find the offset to the right DTB within the FIT image.
> For this it iterates over all subnodes of the /configuration node in
> the FIT tree and compares all "description" strings therein using a
> board specific matching function.
> If that finds a match, it uses the string in the "fdt" property of that
> subnode to locate the matching subnode in the /images node, which points
> to the DTB data.
> Now this works very well, but is quite specific to cover this particular
> use case. To open up the door for a more generic usage, let's split this
> function into:
> 1) a function that just returns the node offset for the matching
>    configuration node (spl_fit_find_config_node())
> 2) a function that returns the image data any given property in a given
>    configuration node points to, additionally using a given index into
>    a possbile list of strings (spl_fit_select_index())
> This allows us to replace the specific function above by asking for the
> image the _first string of the "fdt" property_ in the matching
> configuration subnode points to.
> 
> This patch introduces no functional changes, it just refactors the code
> to allow reusing it later.
> 
> (diff is overly clever here and produces a hard-to-read patch, so I
> recommend to throw a look at the result instead).
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Lokesh Vutla <lokeshvuta@ti.com>

Thanks and regards,
Lokesh

[U-Boot] [RFC PATCH 01/11] SPL: FIT: refactor FDT loading

[Simon Glass]

Hi Andre,

On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote:
>
> Currently the SPL FIT loader uses the spl_fit_select_fdt() function to
> find the offset to the right DTB within the FIT image.
> For this it iterates over all subnodes of the /configuration node in
> the FIT tree and compares all "description" strings therein using a
> board specific matching function.
> If that finds a match, it uses the string in the "fdt" property of that
> subnode to locate the matching subnode in the /images node, which points
> to the DTB data.
> Now this works very well, but is quite specific to cover this particular
> use case. To open up the door for a more generic usage, let's split this
> function into:
> 1) a function that just returns the node offset for the matching
>    configuration node (spl_fit_find_config_node())
> 2) a function that returns the image data any given property in a given
>    configuration node points to, additionally using a given index into
>    a possbile list of strings (spl_fit_select_index())
> This allows us to replace the specific function above by asking for the
> image the _first string of the "fdt" property_ in the matching
> configuration subnode points to.
>
> This patch introduces no functional changes, it just refactors the code
> to allow reusing it later.
>
> (diff is overly clever here and produces a hard-to-read patch, so I
> recommend to throw a look at the result instead).
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  common/spl/spl_fit.c | 82 ++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 51 insertions(+), 31 deletions(-)
>

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

Please check below.

> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index aae556f..c4e2f02 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -22,13 +22,11 @@ static ulong fdt_getprop_u32(const void *fdt, int node, const char *prop)
>         return fdt32_to_cpu(*cell);
>  }
>
> -static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp)
> +static int spl_fit_find_config_node(const void *fdt)
>  {
> -       const char *name, *fdt_name;
> -       int conf, node, fdt_node;
> -       int len;
> +       const char *name;
> +       int conf, node, len;
>
> -       *fdt_offsetp = 0;
>         conf = fdt_path_offset(fdt, FIT_CONFS_PATH);
>         if (conf < 0) {
>                 debug("%s: Cannot find /configurations node: %d\n", __func__,
> @@ -50,39 +48,60 @@ static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp)
>                         continue;
>
>                 debug("Selecting config '%s'", name);
> -               fdt_name = fdt_getprop(fdt, node, FIT_FDT_PROP, &len);
> -               if (!fdt_name) {
> -                       debug("%s: Cannot find fdt name property: %d\n",
> -                             __func__, len);
> -                       return -EINVAL;
> -               }
>
> -               debug(", fdt '%s'\n", fdt_name);
> -               fdt_node = fdt_subnode_offset(fdt, images, fdt_name);
> -               if (fdt_node < 0) {
> -                       debug("%s: Cannot find fdt node '%s': %d\n",
> -                             __func__, fdt_name, fdt_node);
> -                       return -EINVAL;
> +               return node;
> +       }
> +
> +       return -1;
> +}
> +
> +static int spl_fit_select_index(const void *fit, int images, int *offsetp,
> +                               const char *type, int index)
> +{
> +       const char *name, *img_name;
> +       int node, conf_node;
> +       int len, i;
> +
> +       *offsetp = 0;
> +       conf_node = spl_fit_find_config_node(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);
> +                    node >= 0;
> +                    node = fdt_next_subnode(fit, node)) {
> +                       name = fdt_getprop(fit, node, "description", &len);
> +                       printf("   %s\n", name);
>                 }
> +#endif
> +               return -ENOENT;
> +       }
>
> -               *fdt_offsetp = fdt_getprop_u32(fdt, fdt_node, "data-offset");
> -               len = fdt_getprop_u32(fdt, fdt_node, "data-size");
> -               debug("FIT: Selected '%s'\n", name);
> +       img_name = fdt_getprop(fit, conf_node, type, &len);
> +       if (!img_name) {
> +               debug("cannot find property '%s': %d\n", type, len);
> +               return -EINVAL;
> +       }
>
> -               return len;
> +       for (i = 0; i < index; i++) {
> +               img_name = strchr(img_name, '\0') + 1;

Don't you need to check against strchr() returning NULL?

> +               if (*img_name == '\0') {
> +                       debug("no string for index %d\n", index);
> +                       return -E2BIG;
> +               }
>         }
>
> -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> -       printf("No matching DT out of these options:\n");
> -       for (node = fdt_first_subnode(fdt, conf);
> -            node >= 0;
> -            node = fdt_next_subnode(fdt, node)) {
> -               name = fdt_getprop(fdt, node, "description", &len);
> -               printf("   %s\n", name);
> +       debug("%s: '%s'\n", type, img_name);
> +       node = fdt_subnode_offset(fit, images, img_name);
> +       if (node < 0) {
> +               debug("cannot find image node '%s': %d\n", img_name, node);
> +               return -EINVAL;
>         }
> -#endif
>
> -       return -ENOENT;
> +       *offsetp = fdt_getprop_u32(fit, node, "data-offset");
> +       len = fdt_getprop_u32(fit, node, "data-size");
> +
> +       return len;
>  }
>
>  static int get_aligned_image_offset(struct spl_load_info *info, int offset)
> @@ -218,7 +237,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>         memcpy(dst, src, data_size);
>
>         /* Figure out which device tree the board wants to use */
> -       fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset);
> +       fdt_len = spl_fit_select_index(fit, images, &fdt_offset,
> +                                      FIT_FDT_PROP, 0);
>         if (fdt_len < 0)
>                 return fdt_len;
>
> --
> 2.8.2
>

Regards,
Simon

[U-Boot] [RFC PATCH 01/11] SPL: FIT: refactor FDT loading

[André Przywara]

On 27/01/17 21:29, Simon Glass wrote:
> Hi Andre,
> 
> On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> Currently the SPL FIT loader uses the spl_fit_select_fdt() function to
>> find the offset to the right DTB within the FIT image.
>> For this it iterates over all subnodes of the /configuration node in
>> the FIT tree and compares all "description" strings therein using a
>> board specific matching function.
>> If that finds a match, it uses the string in the "fdt" property of that
>> subnode to locate the matching subnode in the /images node, which points
>> to the DTB data.
>> Now this works very well, but is quite specific to cover this particular
>> use case. To open up the door for a more generic usage, let's split this
>> function into:
>> 1) a function that just returns the node offset for the matching
>>    configuration node (spl_fit_find_config_node())
>> 2) a function that returns the image data any given property in a given
>>    configuration node points to, additionally using a given index into
>>    a possbile list of strings (spl_fit_select_index())
>> This allows us to replace the specific function above by asking for the
>> image the _first string of the "fdt" property_ in the matching
>> configuration subnode points to.
>>
>> This patch introduces no functional changes, it just refactors the code
>> to allow reusing it later.
>>
>> (diff is overly clever here and produces a hard-to-read patch, so I
>> recommend to throw a look at the result instead).
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  common/spl/spl_fit.c | 82 ++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 51 insertions(+), 31 deletions(-)
>>
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

Thanks a lot!

> 
> Please check below.

....

>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index aae556f..c4e2f02 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -22,13 +22,11 @@ static ulong fdt_getprop_u32(const void *fdt, int node, const char *prop)
>>         return fdt32_to_cpu(*cell);
>>  }
>>
>> -static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp)
>> +static int spl_fit_find_config_node(const void *fdt)
>>  {
>> -       const char *name, *fdt_name;
>> -       int conf, node, fdt_node;
>> -       int len;
>> +       const char *name;
>> +       int conf, node, len;
>>
>> -       *fdt_offsetp = 0;
>>         conf = fdt_path_offset(fdt, FIT_CONFS_PATH);
>>         if (conf < 0) {
>>                 debug("%s: Cannot find /configurations node: %d\n", __func__,
>> @@ -50,39 +48,60 @@ static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp)
>>                         continue;
>>
>>                 debug("Selecting config '%s'", name);
>> -               fdt_name = fdt_getprop(fdt, node, FIT_FDT_PROP, &len);
>> -               if (!fdt_name) {
>> -                       debug("%s: Cannot find fdt name property: %d\n",
>> -                             __func__, len);
>> -                       return -EINVAL;
>> -               }
>>
>> -               debug(", fdt '%s'\n", fdt_name);
>> -               fdt_node = fdt_subnode_offset(fdt, images, fdt_name);
>> -               if (fdt_node < 0) {
>> -                       debug("%s: Cannot find fdt node '%s': %d\n",
>> -                             __func__, fdt_name, fdt_node);
>> -                       return -EINVAL;
>> +               return node;
>> +       }
>> +
>> +       return -1;
>> +}
>> +
>> +static int spl_fit_select_index(const void *fit, int images, int *offsetp,
>> +                               const char *type, int index)
>> +{
>> +       const char *name, *img_name;
>> +       int node, conf_node;
>> +       int len, i;
>> +
>> +       *offsetp = 0;
>> +       conf_node = spl_fit_find_config_node(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);
>> +                    node >= 0;
>> +                    node = fdt_next_subnode(fit, node)) {
>> +                       name = fdt_getprop(fit, node, "description", &len);
>> +                       printf("   %s\n", name);
>>                 }
>> +#endif
>> +               return -ENOENT;
>> +       }
>>
>> -               *fdt_offsetp = fdt_getprop_u32(fdt, fdt_node, "data-offset");
>> -               len = fdt_getprop_u32(fdt, fdt_node, "data-size");
>> -               debug("FIT: Selected '%s'\n", name);
>> +       img_name = fdt_getprop(fit, conf_node, type, &len);
>> +       if (!img_name) {
>> +               debug("cannot find property '%s': %d\n", type, len);
>> +               return -EINVAL;
>> +       }
>>
>> -               return len;
>> +       for (i = 0; i < index; i++) {
>> +               img_name = strchr(img_name, '\0') + 1;
> 
> Don't you need to check against strchr() returning NULL?

I think in a valid DT a string is always terminated by a NUL, but you
are right that this code is broken. There is no double NUL at the end as
I erroneously thought, instead we have to compare against the "len"
value from above.

Thanks for spotting this!

Cheers,
Andre.

>> +               if (*img_name == '\0') {
>> +                       debug("no string for index %d\n", index);
>> +                       return -E2BIG;
>> +               }
>>         }
>>
>> -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>> -       printf("No matching DT out of these options:\n");
>> -       for (node = fdt_first_subnode(fdt, conf);
>> -            node >= 0;
>> -            node = fdt_next_subnode(fdt, node)) {
>> -               name = fdt_getprop(fdt, node, "description", &len);
>> -               printf("   %s\n", name);
>> +       debug("%s: '%s'\n", type, img_name);
>> +       node = fdt_subnode_offset(fit, images, img_name);
>> +       if (node < 0) {
>> +               debug("cannot find image node '%s': %d\n", img_name, node);
>> +               return -EINVAL;
>>         }
>> -#endif
>>
>> -       return -ENOENT;
>> +       *offsetp = fdt_getprop_u32(fit, node, "data-offset");
>> +       len = fdt_getprop_u32(fit, node, "data-size");
>> +
>> +       return len;
>>  }
>>
>>  static int get_aligned_image_offset(struct spl_load_info *info, int offset)
>> @@ -218,7 +237,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>>         memcpy(dst, src, data_size);
>>
>>         /* Figure out which device tree the board wants to use */
>> -       fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset);
>> +       fdt_len = spl_fit_select_index(fit, images, &fdt_offset,
>> +                                      FIT_FDT_PROP, 0);
>>         if (fdt_len < 0)
>>                 return fdt_len;
>>
>> --
>> 2.8.2
>>
> 
> Regards,
> Simon
> 

[U-Boot] [RFC PATCH 02/11] SPL: FIT: rework U-Boot image loading

[Andre Przywara]

Currently the SPL FIT loader always looks only for the first image in
the /images node a FIT tree, which it loads and later executes.

Generalize this by looking for a "firmware" property in the matched
configuration subnode, or, if that does not exist, for the first string
in the "loadables" property. Then using the string in that property,
load the image of that name from the /images node.
This still loads only one image at the moment, but refactors the code to
allow extending this in a following patch.
To simplify later re-usage, we also generalize the spl_fit_select_index()
function to not return the image location, but just the node offset.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 common/spl/spl_fit.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index c4e2f02..381ed1f 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -55,14 +55,13 @@ static int spl_fit_find_config_node(const void *fdt)
 	return -1;
 }
 
-static int spl_fit_select_index(const void *fit, int images, int *offsetp,
-				const char *type, int index)
+static int spl_fit_get_image_node(const void *fit, int images,
+				  const char *type, int index)
 {
 	const char *name, *img_name;
 	int node, conf_node;
 	int len, i;
 
-	*offsetp = 0;
 	conf_node = spl_fit_find_config_node(fit);
 	if (conf_node < 0) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
@@ -98,10 +97,7 @@ static int spl_fit_select_index(const void *fit, int images, int *offsetp,
 		return -EINVAL;
 	}
 
-	*offsetp = fdt_getprop_u32(fit, node, "data-offset");
-	len = fdt_getprop_u32(fit, node, "data-size");
-
-	return len;
+	return node;
 }
 
 static int get_aligned_image_offset(struct spl_load_info *info, int offset)
@@ -187,15 +183,22 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	if (count == 0)
 		return -EIO;
 
-	/* find the firmware image to load */
+	/* 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;
 	}
-	node = fdt_first_subnode(fit, images);
+
+	/* find the U-Boot image */
+	node = spl_fit_get_image_node(fit, images, "firmware", 0);
+	if (node < 0) {
+		debug("could not find firmware image, trying loadables...\n");
+		node = spl_fit_get_image_node(fit, images, "loadables", 0);
+	}
 	if (node < 0) {
-		debug("%s: Cannot find first image node: %d\n", __func__, node);
+		debug("%s: Cannot find u-boot image node: %d\n",
+		      __func__, node);
 		return -1;
 	}
 
@@ -237,10 +240,13 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	memcpy(dst, src, data_size);
 
 	/* Figure out which device tree the board wants to use */
-	fdt_len = spl_fit_select_index(fit, images, &fdt_offset,
-				       FIT_FDT_PROP, 0);
-	if (fdt_len < 0)
-		return fdt_len;
+	node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, 0);
+	if (node < 0) {
+		debug("%s: cannot find FDT node\n", __func__);
+		return node;
+	}
+	fdt_offset = fdt_getprop_u32(fit, node, "data-offset");
+	fdt_len = fdt_getprop_u32(fit, node, "data-size");
 
 	/*
 	 * Read the device tree and place it after the image. There may be
-- 
2.8.2

[U-Boot] [RFC PATCH 02/11] SPL: FIT: rework U-Boot image loading

[Lokesh Vutla]

On Friday 20 January 2017 07:23 AM, Andre Przywara wrote:
> Currently the SPL FIT loader always looks only for the first image in
> the /images node a FIT tree, which it loads and later executes.
> 
> Generalize this by looking for a "firmware" property in the matched
> configuration subnode, or, if that does not exist, for the first string
> in the "loadables" property. Then using the string in that property,
> load the image of that name from the /images node.
> This still loads only one image at the moment, but refactors the code to
> allow extending this in a following patch.
> To simplify later re-usage, we also generalize the spl_fit_select_index()
> function to not return the image location, but just the node offset.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Lokesh Vutla <lokeshvuta@ti.com>

Thanks and regards,
Lokesh

[U-Boot] [RFC PATCH 02/11] SPL: FIT: rework U-Boot image loading

[Simon Glass]

On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote:
> Currently the SPL FIT loader always looks only for the first image in
> the /images node a FIT tree, which it loads and later executes.
>
> Generalize this by looking for a "firmware" property in the matched
> configuration subnode, or, if that does not exist, for the first string
> in the "loadables" property. Then using the string in that property,
> load the image of that name from the /images node.
> This still loads only one image at the moment, but refactors the code to
> allow extending this in a following patch.
> To simplify later re-usage, we also generalize the spl_fit_select_index()
> function to not return the image location, but just the node offset.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  common/spl/spl_fit.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)

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

[U-Boot] [RFC PATCH 03/11] SPL: FIT: factor out spl_load_fit_image()

[Andre Przywara]

At the moment we load two images from a FIT image: the actual U-Boot
image and the DTB. Both times we have very similar code to deal with
alignment requirement the media we load from imposes upon us.
Factor out this code into a new function, which we just call twice.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 common/spl/spl_fit.c | 122 +++++++++++++++++++++------------------------------
 1 file changed, 51 insertions(+), 71 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 381ed1f..d4149c5 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -138,19 +138,58 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
 	return (data_size + info->bl_len - 1) / info->bl_len;
 }
 
+static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
+			      void *fit, ulong base_offset, int node,
+			      struct spl_image_info *image_info)
+{
+	ulong offset;
+	size_t length;
+	ulong load, entry;
+	void *src;
+	ulong overhead;
+	int nr_sectors;
+
+	offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
+	length = fdt_getprop_u32(fit, node, "data-size");
+	load = fdt_getprop_u32(fit, node, "load");
+	if (load == -1U && image_info)
+		load = image_info->load_addr;
+	entry = fdt_getprop_u32(fit, node, "entry");
+
+	overhead = get_aligned_image_overhead(info, offset);
+	nr_sectors = get_aligned_image_size(info, overhead + length, offset);
+
+	if (info->read(info, sector + get_aligned_image_offset(info, offset),
+		       nr_sectors, (void*)load) != nr_sectors)
+		return -EIO;
+	debug("image: dst=%lx, offset=%lx, size=%lx\n", load, offset,
+	      (unsigned long)length);
+
+	src = (void *)load + overhead;
+#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
+	board_fit_image_post_process(&src, &length);
+#endif
+
+	memcpy((void*)load, src, length);
+
+	if (image_info) {
+		image_info->load_addr = load;
+		image_info->size = length;
+		image_info->entry_point = entry;
+	}
+
+	return 0;
+}
+
 int spl_load_simple_fit(struct spl_image_info *spl_image,
 			struct spl_load_info *info, ulong sector, void *fit)
 {
 	int sectors;
-	ulong size, load;
+	ulong size;
 	unsigned long count;
+	struct spl_image_info image_info;
 	int node, images;
-	void *load_ptr;
-	int fdt_offset, fdt_len;
-	int data_offset, data_size;
 	int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
-	int src_sector;
-	void *dst, *src;
 
 	/*
 	 * Figure out where the external images start. This is the base for the
@@ -202,82 +241,23 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 		return -1;
 	}
 
-	/* Get its information and set up the spl_image structure */
-	data_offset = fdt_getprop_u32(fit, node, "data-offset");
-	data_size = fdt_getprop_u32(fit, node, "data-size");
-	load = fdt_getprop_u32(fit, node, "load");
-	debug("data_offset=%x, data_size=%x\n", data_offset, data_size);
-	spl_image->load_addr = load;
-	spl_image->entry_point = load;
+	/* Load the image and set up the spl_image structure */
+	spl_load_fit_image(info, sector, fit, base_offset, node, spl_image);
 	spl_image->os = IH_OS_U_BOOT;
 
-	/*
-	 * Work out where to place the image. We read it so that the first
-	 * byte will be at 'load'. This may mean we need to load it starting
-	 * before then, since we can only read whole blocks.
-	 */
-	data_offset += base_offset;
-	sectors = get_aligned_image_size(info, data_size, data_offset);
-	load_ptr = (void *)load;
-	debug("U-Boot size %x, data %p\n", data_size, load_ptr);
-	dst = load_ptr;
-
-	/* Read the image */
-	src_sector = sector + get_aligned_image_offset(info, data_offset);
-	debug("Aligned image read: dst=%p, src_sector=%x, sectors=%x\n",
-	      dst, src_sector, sectors);
-	count = info->read(info, src_sector, sectors, dst);
-	if (count != sectors)
-		return -EIO;
-	debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,
-	      data_size);
-	src = dst + get_aligned_image_overhead(info, data_offset);
-
-#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
-	board_fit_image_post_process((void **)&src, (size_t *)&data_size);
-#endif
-
-	memcpy(dst, src, data_size);
-
 	/* Figure out which device tree the board wants to use */
 	node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, 0);
 	if (node < 0) {
 		debug("%s: cannot find FDT node\n", __func__);
 		return node;
 	}
-	fdt_offset = fdt_getprop_u32(fit, node, "data-offset");
-	fdt_len = fdt_getprop_u32(fit, node, "data-size");
 
 	/*
-	 * Read the device tree and place it after the image. There may be
-	 * some extra data before it since we can only read entire blocks.
-	 * And also align the destination address to ARCH_DMA_MINALIGN.
+	 * Read the device tree and place it after the image.
+	 * Align the destination address to ARCH_DMA_MINALIGN.
 	 */
-	dst = (void *)((load + data_size + align_len) & ~align_len);
-	fdt_offset += base_offset;
-	sectors = get_aligned_image_size(info, fdt_len, fdt_offset);
-	src_sector = sector + get_aligned_image_offset(info, fdt_offset);
-	count = info->read(info, src_sector, sectors, dst);
-	debug("Aligned fdt read: dst %p, src_sector = %x, sectors %x\n",
-	      dst, src_sector, sectors);
-	if (count != sectors)
-		return -EIO;
-
-	/*
-	 * Copy the device tree so that it starts immediately after the image.
-	 * After this we will have the U-Boot image and its device tree ready
-	 * for us to start.
-	 */
-	debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,
-	      fdt_len);
-	src = dst + get_aligned_image_overhead(info, fdt_offset);
-	dst = load_ptr + data_size;
-
-#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
-	board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);
-#endif
-
-	memcpy(dst, src, fdt_len);
+	image_info.load_addr = spl_image->load_addr + spl_image->size;
+	spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);
 
 	return 0;
 }
-- 
2.8.2

[U-Boot] [RFC PATCH 03/11] SPL: FIT: factor out spl_load_fit_image()

[Kever Yang]

Hi Andre,

On 01/20/2017 09:53 AM, Andre Przywara wrote:
> At the moment we load two images from a FIT image: the actual U-Boot
> image and the DTB. Both times we have very similar code to deal with
> alignment requirement the media we load from imposes upon us.
> Factor out this code into a new function, which we just call twice.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>   common/spl/spl_fit.c | 122 +++++++++++++++++++++------------------------------
>   1 file changed, 51 insertions(+), 71 deletions(-)
>
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 381ed1f..d4149c5 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
>   	return (data_size + info->bl_len - 1) / info->bl_len;
>   }
>   
> +static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
> +			      void *fit, ulong base_offset, int node,
> +			      struct spl_image_info *image_info)
> +{
> +	ulong offset;
> +	size_t length;
> +	ulong load, entry;
> +	void *src;
> +	ulong overhead;
> +	int nr_sectors;
> +
> +	offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
> +	length = fdt_getprop_u32(fit, node, "data-size");
> +	load = fdt_getprop_u32(fit, node, "load");
> +	if (load == -1U && image_info)
> +		load = image_info->load_addr;
> +	entry = fdt_getprop_u32(fit, node, "entry");
> +
> +	overhead = get_aligned_image_overhead(info, offset);
> +	nr_sectors = get_aligned_image_size(info, overhead + length, offset);
> +
> +	if (info->read(info, sector + get_aligned_image_offset(info, offset),
> +		       nr_sectors, (void*)load) != nr_sectors)
> +		return -EIO;
> +	debug("image: dst=%lx, offset=%lx, size=%lx\n", load, offset,
> +	      (unsigned long)length);
> +
> +	src = (void *)load + overhead;
> +#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
> +	board_fit_image_post_process(&src, &length);
> +#endif
> +
> +	memcpy((void*)load, src, length);

For FIT image, we read the first block for header, and then we know the 
offset
and size for images, is it possible to have an option that we can read 
the image
data to load address directly without memcpy(), this helps speed up the 
boot time.

Thanks,
- Kever
> +
> +	if (image_info) {
> +		image_info->load_addr = load;
> +		image_info->size = length;
> +		image_info->entry_point = entry;
> +	}
> +
> +	return 0;
> +}
> +
>   int spl_load_simple_fit(struct spl_image_info *spl_image,
>   			struct spl_load_info *info, ulong sector, void *fit)
>   {
>   	int sectors;
> -	ulong size, load;
> +	ulong size;
>   	unsigned long count;
> +	struct spl_image_info image_info;
>   	int node, images;
> -	void *load_ptr;
> -	int fdt_offset, fdt_len;
> -	int data_offset, data_size;
>   	int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
> -	int src_sector;
> -	void *dst, *src;
>   
>   	/*
>   	 * Figure out where the external images start. This is the base for the
> @@ -202,82 +241,23 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>   		return -1;
>   	}
>   
> -	/* Get its information and set up the spl_image structure */
> -	data_offset = fdt_getprop_u32(fit, node, "data-offset");
> -	data_size = fdt_getprop_u32(fit, node, "data-size");
> -	load = fdt_getprop_u32(fit, node, "load");
> -	debug("data_offset=%x, data_size=%x\n", data_offset, data_size);
> -	spl_image->load_addr = load;
> -	spl_image->entry_point = load;
> +	/* Load the image and set up the spl_image structure */
> +	spl_load_fit_image(info, sector, fit, base_offset, node, spl_image);
>   	spl_image->os = IH_OS_U_BOOT;
>   
> -	/*
> -	 * Work out where to place the image. We read it so that the first
> -	 * byte will be at 'load'. This may mean we need to load it starting
> -	 * before then, since we can only read whole blocks.
> -	 */
> -	data_offset += base_offset;
> -	sectors = get_aligned_image_size(info, data_size, data_offset);
> -	load_ptr = (void *)load;
> -	debug("U-Boot size %x, data %p\n", data_size, load_ptr);
> -	dst = load_ptr;
> -
> -	/* Read the image */
> -	src_sector = sector + get_aligned_image_offset(info, data_offset);
> -	debug("Aligned image read: dst=%p, src_sector=%x, sectors=%x\n",
> -	      dst, src_sector, sectors);
> -	count = info->read(info, src_sector, sectors, dst);
> -	if (count != sectors)
> -		return -EIO;
> -	debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,
> -	      data_size);
> -	src = dst + get_aligned_image_overhead(info, data_offset);
> -
> -#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
> -	board_fit_image_post_process((void **)&src, (size_t *)&data_size);
> -#endif
> -
> -	memcpy(dst, src, data_size);
> -
>   	/* Figure out which device tree the board wants to use */
>   	node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, 0);
>   	if (node < 0) {
>   		debug("%s: cannot find FDT node\n", __func__);
>   		return node;
>   	}
> -	fdt_offset = fdt_getprop_u32(fit, node, "data-offset");
> -	fdt_len = fdt_getprop_u32(fit, node, "data-size");
>   
>   	/*
> -	 * Read the device tree and place it after the image. There may be
> -	 * some extra data before it since we can only read entire blocks.
> -	 * And also align the destination address to ARCH_DMA_MINALIGN.
> +	 * Read the device tree and place it after the image.
> +	 * Align the destination address to ARCH_DMA_MINALIGN.
>   	 */
> -	dst = (void *)((load + data_size + align_len) & ~align_len);
> -	fdt_offset += base_offset;
> -	sectors = get_aligned_image_size(info, fdt_len, fdt_offset);
> -	src_sector = sector + get_aligned_image_offset(info, fdt_offset);
> -	count = info->read(info, src_sector, sectors, dst);
> -	debug("Aligned fdt read: dst %p, src_sector = %x, sectors %x\n",
> -	      dst, src_sector, sectors);
> -	if (count != sectors)
> -		return -EIO;
> -
> -	/*
> -	 * Copy the device tree so that it starts immediately after the image.
> -	 * After this we will have the U-Boot image and its device tree ready
> -	 * for us to start.
> -	 */
> -	debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,
> -	      fdt_len);
> -	src = dst + get_aligned_image_overhead(info, fdt_offset);
> -	dst = load_ptr + data_size;
> -
> -#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
> -	board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);
> -#endif
> -
> -	memcpy(dst, src, fdt_len);
> +	image_info.load_addr = spl_image->load_addr + spl_image->size;
> +	spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);
>   
>   	return 0;
>   }

[U-Boot] [RFC PATCH 03/11] SPL: FIT: factor out spl_load_fit_image()

[André Przywara]

On 22/01/17 07:16, Kever Yang wrote:
> Hi Andre,
> 
> On 01/20/2017 09:53 AM, Andre Przywara wrote:
>> At the moment we load two images from a FIT image: the actual U-Boot
>> image and the DTB. Both times we have very similar code to deal with
>> alignment requirement the media we load from imposes upon us.
>> Factor out this code into a new function, which we just call twice.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>   common/spl/spl_fit.c | 122
>> +++++++++++++++++++++------------------------------
>>   1 file changed, 51 insertions(+), 71 deletions(-)
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index 381ed1f..d4149c5 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct
>> spl_load_info *info, int data_size,
>>       return (data_size + info->bl_len - 1) / info->bl_len;
>>   }
>>   +static int spl_load_fit_image(struct spl_load_info *info, ulong
>> sector,
>> +                  void *fit, ulong base_offset, int node,
>> +                  struct spl_image_info *image_info)
>> +{
>> +    ulong offset;
>> +    size_t length;
>> +    ulong load, entry;
>> +    void *src;
>> +    ulong overhead;
>> +    int nr_sectors;
>> +
>> +    offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
>> +    length = fdt_getprop_u32(fit, node, "data-size");
>> +    load = fdt_getprop_u32(fit, node, "load");
>> +    if (load == -1U && image_info)
>> +        load = image_info->load_addr;

	    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +    entry = fdt_getprop_u32(fit, node, "entry");
>> +
>> +    overhead = get_aligned_image_overhead(info, offset);
>> +    nr_sectors = get_aligned_image_size(info, overhead + length,
>> offset);
>> +
>> +    if (info->read(info, sector + get_aligned_image_offset(info,
>> offset),
>> +               nr_sectors, (void*)load) != nr_sectors)
>> +        return -EIO;
>> +    debug("image: dst=%lx, offset=%lx, size=%lx\n", load, offset,
>> +          (unsigned long)length);
>> +
>> +    src = (void *)load + overhead;
>> +#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
>> +    board_fit_image_post_process(&src, &length);
>> +#endif
>> +
>> +    memcpy((void*)load, src, length);
> 
> For FIT image, we read the first block for header, and then we know the
> offset
> and size for images, is it possible to have an option that we can read
> the image
> data to load address directly without memcpy(), this helps speed up the
> boot time.

We do read directly into the load address (see the marked place above).
But we have to fix up the alignment:
a) The load address may not be cache line aligned. I am not totally
convinced we really need this, but the code takes care of this.
Shouldn't be an issue in practice since most load addresses are already
aligned.
b) More importantly we need to observe the block size of the device we
read from. mkimage packs the images as tightly as possible into the
image, so ATF may start at say offset 0x456 within the FIT image. Given
that the FIT image itself starts at a sector boundary, that may be "in
the middle" of a sector, which is the smallest addressable unit for
block devices. For MMC based storage for instance this is typically 512
bytes. So we read the whole sector and later discard the overhead by
copying the image back in memory.

Questions to you:
1) Is the memcpy really an issue? I take it you use bl31.bin only, which
is probably between 32KB and 64KB. So does the memcpy really contribute
to boot time here?
2) What device do you usually read from? What is the alignment there?
For instance SPI flash has typically an alignment of 1 Byte, so no
adjustment is actually needed. The memcpy routine bails out if dst==src,
so in this case there are no cycles spend on this.
3) Can you tweak mkimage to observe alignment? As I said above, in the
moment it packs it tightly, but I don't see why we couldn't align
everything in the image at the cost of loosing 511 Bytes per embedded
image in the worst case, for instance. This would eventually fold into
the same point as question 2): If everything is aligned already, memcpy
is a NOP.

So one relatively easy and clean solution would be to introduce an
alignment command line option to mkimage. One could optionally specify a
value, to which mkimage would align each embedded images into, leaving
gaps in between. We would still go over all the calculations in the SPL
code, but all the offsets and corrections would end up being 0, so no
copying would be necessary anymore.

Does this sound like a plan?

Cheers,
Andre.

>> +
>> +    if (image_info) {
>> +        image_info->load_addr = load;
>> +        image_info->size = length;
>> +        image_info->entry_point = entry;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   int spl_load_simple_fit(struct spl_image_info *spl_image,
>>               struct spl_load_info *info, ulong sector, void *fit)
>>   {
>>       int sectors;
>> -    ulong size, load;
>> +    ulong size;
>>       unsigned long count;
>> +    struct spl_image_info image_info;
>>       int node, images;
>> -    void *load_ptr;
>> -    int fdt_offset, fdt_len;
>> -    int data_offset, data_size;
>>       int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
>> -    int src_sector;
>> -    void *dst, *src;
>>         /*
>>        * Figure out where the external images start. This is the base
>> for the
>> @@ -202,82 +241,23 @@ int spl_load_simple_fit(struct spl_image_info
>> *spl_image,
>>           return -1;
>>       }
>>   -    /* Get its information and set up the spl_image structure */
>> -    data_offset = fdt_getprop_u32(fit, node, "data-offset");
>> -    data_size = fdt_getprop_u32(fit, node, "data-size");
>> -    load = fdt_getprop_u32(fit, node, "load");
>> -    debug("data_offset=%x, data_size=%x\n", data_offset, data_size);
>> -    spl_image->load_addr = load;
>> -    spl_image->entry_point = load;
>> +    /* Load the image and set up the spl_image structure */
>> +    spl_load_fit_image(info, sector, fit, base_offset, node, spl_image);
>>       spl_image->os = IH_OS_U_BOOT;
>>   -    /*
>> -     * Work out where to place the image. We read it so that the first
>> -     * byte will be at 'load'. This may mean we need to load it starting
>> -     * before then, since we can only read whole blocks.
>> -     */
>> -    data_offset += base_offset;
>> -    sectors = get_aligned_image_size(info, data_size, data_offset);
>> -    load_ptr = (void *)load;
>> -    debug("U-Boot size %x, data %p\n", data_size, load_ptr);
>> -    dst = load_ptr;
>> -
>> -    /* Read the image */
>> -    src_sector = sector + get_aligned_image_offset(info, data_offset);
>> -    debug("Aligned image read: dst=%p, src_sector=%x, sectors=%x\n",
>> -          dst, src_sector, sectors);
>> -    count = info->read(info, src_sector, sectors, dst);
>> -    if (count != sectors)
>> -        return -EIO;
>> -    debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,
>> -          data_size);
>> -    src = dst + get_aligned_image_overhead(info, data_offset);
>> -
>> -#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
>> -    board_fit_image_post_process((void **)&src, (size_t *)&data_size);
>> -#endif
>> -
>> -    memcpy(dst, src, data_size);
>> -
>>       /* Figure out which device tree the board wants to use */
>>       node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, 0);
>>       if (node < 0) {
>>           debug("%s: cannot find FDT node\n", __func__);
>>           return node;
>>       }
>> -    fdt_offset = fdt_getprop_u32(fit, node, "data-offset");
>> -    fdt_len = fdt_getprop_u32(fit, node, "data-size");
>>         /*
>> -     * Read the device tree and place it after the image. There may be
>> -     * some extra data before it since we can only read entire blocks.
>> -     * And also align the destination address to ARCH_DMA_MINALIGN.
>> +     * Read the device tree and place it after the image.
>> +     * Align the destination address to ARCH_DMA_MINALIGN.
>>        */
>> -    dst = (void *)((load + data_size + align_len) & ~align_len);
>> -    fdt_offset += base_offset;
>> -    sectors = get_aligned_image_size(info, fdt_len, fdt_offset);
>> -    src_sector = sector + get_aligned_image_offset(info, fdt_offset);
>> -    count = info->read(info, src_sector, sectors, dst);
>> -    debug("Aligned fdt read: dst %p, src_sector = %x, sectors %x\n",
>> -          dst, src_sector, sectors);
>> -    if (count != sectors)
>> -        return -EIO;
>> -
>> -    /*
>> -     * Copy the device tree so that it starts immediately after the
>> image.
>> -     * After this we will have the U-Boot image and its device tree
>> ready
>> -     * for us to start.
>> -     */
>> -    debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,
>> -          fdt_len);
>> -    src = dst + get_aligned_image_overhead(info, fdt_offset);
>> -    dst = load_ptr + data_size;
>> -
>> -#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
>> -    board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);
>> -#endif
>> -
>> -    memcpy(dst, src, fdt_len);
>> +    image_info.load_addr = spl_image->load_addr + spl_image->size;
>> +    spl_load_fit_image(info, sector, fit, base_offset, node,
>> &image_info);
>>         return 0;
>>   }
> 
> 

[U-Boot] [RFC PATCH 03/11] SPL: FIT: factor out spl_load_fit_image()

[Kever Yang]

Hi Andre,

On 01/22/2017 06:42 PM, Andr? Przywara wrote:
> On 22/01/17 07:16, Kever Yang wrote:
>> Hi Andre,
>>
>> On 01/20/2017 09:53 AM, Andre Przywara wrote:
>>> At the moment we load two images from a FIT image: the actual U-Boot
>>> image and the DTB. Both times we have very similar code to deal with
>>> alignment requirement the media we load from imposes upon us.
>>> Factor out this code into a new function, which we just call twice.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>    common/spl/spl_fit.c | 122
>>> +++++++++++++++++++++------------------------------
>>>    1 file changed, 51 insertions(+), 71 deletions(-)
>>>
>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>> index 381ed1f..d4149c5 100644
>>> --- a/common/spl/spl_fit.c
>>> +++ b/common/spl/spl_fit.c
>>> @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct
>>> spl_load_info *info, int data_size,
>>>        return (data_size + info->bl_len - 1) / info->bl_len;
>>>    }
>>>    +static int spl_load_fit_image(struct spl_load_info *info, ulong
>>> sector,
>>> +                  void *fit, ulong base_offset, int node,
>>> +                  struct spl_image_info *image_info)
>>> +{
>>> +    ulong offset;
>>> +    size_t length;
>>> +    ulong load, entry;
>>> +    void *src;
>>> +    ulong overhead;
>>> +    int nr_sectors;
>>> +
>>> +    offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
>>> +    length = fdt_getprop_u32(fit, node, "data-size");
>>> +    load = fdt_getprop_u32(fit, node, "load");
>>> +    if (load == -1U && image_info)
>>> +        load = image_info->load_addr;
> 	    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> +    entry = fdt_getprop_u32(fit, node, "entry");
>>> +
>>> +    overhead = get_aligned_image_overhead(info, offset);
>>> +    nr_sectors = get_aligned_image_size(info, overhead + length,
>>> offset);
>>> +
>>> +    if (info->read(info, sector + get_aligned_image_offset(info,
>>> offset),
>>> +               nr_sectors, (void*)load) != nr_sectors)
>>> +        return -EIO;
>>> +    debug("image: dst=%lx, offset=%lx, size=%lx\n", load, offset,
>>> +          (unsigned long)length);
>>> +
>>> +    src = (void *)load + overhead;
>>> +#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
>>> +    board_fit_image_post_process(&src, &length);
>>> +#endif
>>> +
>>> +    memcpy((void*)load, src, length);
>> For FIT image, we read the first block for header, and then we know the
>> offset
>> and size for images, is it possible to have an option that we can read
>> the image
>> data to load address directly without memcpy(), this helps speed up the
>> boot time.
> We do read directly into the load address (see the marked place above).
> But we have to fix up the alignment:
> a) The load address may not be cache line aligned. I am not totally
> convinced we really need this, but the code takes care of this.
> Shouldn't be an issue in practice since most load addresses are already
> aligned.
> b) More importantly we need to observe the block size of the device we
> read from. mkimage packs the images as tightly as possible into the
> image, so ATF may start at say offset 0x456 within the FIT image. Given
> that the FIT image itself starts at a sector boundary, that may be "in
> the middle" of a sector, which is the smallest addressable unit for
> block devices. For MMC based storage for instance this is typically 512
> bytes. So we read the whole sector and later discard the overhead by
> copying the image back in memory.
>
> Questions to you:
> 1) Is the memcpy really an issue? I take it you use bl31.bin only, which
> is probably between 32KB and 64KB. So does the memcpy really contribute
> to boot time here?
> 2) What device do you usually read from? What is the alignment there?
> For instance SPI flash has typically an alignment of 1 Byte, so no
> adjustment is actually needed. The memcpy routine bails out if dst==src,
> so in this case there are no cycles spend on this.
> 3) Can you tweak mkimage to observe alignment? As I said above, in the
> moment it packs it tightly, but I don't see why we couldn't align
> everything in the image at the cost of loosing 511 Bytes per embedded
> image in the worst case, for instance. This would eventually fold into
> the same point as question 2): If everything is aligned already, memcpy
> is a NOP.
>
> So one relatively easy and clean solution would be to introduce an
> alignment command line option to mkimage. One could optionally specify a
> value, to which mkimage would align each embedded images into, leaving
> gaps in between. We would still go over all the calculations in the SPL
> code, but all the offsets and corrections would end up being 0, so no
> copying would be necessary anymore.

This looks good to me, I didn't notice that there is no copy if dst==src.

Reviewed-by: Kever Yang<kever.yang@rock-chips.com>

Thanks,
- Kever
>
> Does this sound like a plan?
>
> Cheers,
> Andre.
>
>>> +
>>> +    if (image_info) {
>>> +        image_info->load_addr = load;
>>> +        image_info->size = length;
>>> +        image_info->entry_point = entry;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>    int spl_load_simple_fit(struct spl_image_info *spl_image,
>>>                struct spl_load_info *info, ulong sector, void *fit)
>>>    {
>>>        int sectors;
>>> -    ulong size, load;
>>> +    ulong size;
>>>        unsigned long count;
>>> +    struct spl_image_info image_info;
>>>        int node, images;
>>> -    void *load_ptr;
>>> -    int fdt_offset, fdt_len;
>>> -    int data_offset, data_size;
>>>        int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
>>> -    int src_sector;
>>> -    void *dst, *src;
>>>          /*
>>>         * Figure out where the external images start. This is the base
>>> for the
>>> @@ -202,82 +241,23 @@ int spl_load_simple_fit(struct spl_image_info
>>> *spl_image,
>>>            return -1;
>>>        }
>>>    -    /* Get its information and set up the spl_image structure */
>>> -    data_offset = fdt_getprop_u32(fit, node, "data-offset");
>>> -    data_size = fdt_getprop_u32(fit, node, "data-size");
>>> -    load = fdt_getprop_u32(fit, node, "load");
>>> -    debug("data_offset=%x, data_size=%x\n", data_offset, data_size);
>>> -    spl_image->load_addr = load;
>>> -    spl_image->entry_point = load;
>>> +    /* Load the image and set up the spl_image structure */
>>> +    spl_load_fit_image(info, sector, fit, base_offset, node, spl_image);
>>>        spl_image->os = IH_OS_U_BOOT;
>>>    -    /*
>>> -     * Work out where to place the image. We read it so that the first
>>> -     * byte will be at 'load'. This may mean we need to load it starting
>>> -     * before then, since we can only read whole blocks.
>>> -     */
>>> -    data_offset += base_offset;
>>> -    sectors = get_aligned_image_size(info, data_size, data_offset);
>>> -    load_ptr = (void *)load;
>>> -    debug("U-Boot size %x, data %p\n", data_size, load_ptr);
>>> -    dst = load_ptr;
>>> -
>>> -    /* Read the image */
>>> -    src_sector = sector + get_aligned_image_offset(info, data_offset);
>>> -    debug("Aligned image read: dst=%p, src_sector=%x, sectors=%x\n",
>>> -          dst, src_sector, sectors);
>>> -    count = info->read(info, src_sector, sectors, dst);
>>> -    if (count != sectors)
>>> -        return -EIO;
>>> -    debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,
>>> -          data_size);
>>> -    src = dst + get_aligned_image_overhead(info, data_offset);
>>> -
>>> -#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
>>> -    board_fit_image_post_process((void **)&src, (size_t *)&data_size);
>>> -#endif
>>> -
>>> -    memcpy(dst, src, data_size);
>>> -
>>>        /* Figure out which device tree the board wants to use */
>>>        node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, 0);
>>>        if (node < 0) {
>>>            debug("%s: cannot find FDT node\n", __func__);
>>>            return node;
>>>        }
>>> -    fdt_offset = fdt_getprop_u32(fit, node, "data-offset");
>>> -    fdt_len = fdt_getprop_u32(fit, node, "data-size");
>>>          /*
>>> -     * Read the device tree and place it after the image. There may be
>>> -     * some extra data before it since we can only read entire blocks.
>>> -     * And also align the destination address to ARCH_DMA_MINALIGN.
>>> +     * Read the device tree and place it after the image.
>>> +     * Align the destination address to ARCH_DMA_MINALIGN.
>>>         */
>>> -    dst = (void *)((load + data_size + align_len) & ~align_len);
>>> -    fdt_offset += base_offset;
>>> -    sectors = get_aligned_image_size(info, fdt_len, fdt_offset);
>>> -    src_sector = sector + get_aligned_image_offset(info, fdt_offset);
>>> -    count = info->read(info, src_sector, sectors, dst);
>>> -    debug("Aligned fdt read: dst %p, src_sector = %x, sectors %x\n",
>>> -          dst, src_sector, sectors);
>>> -    if (count != sectors)
>>> -        return -EIO;
>>> -
>>> -    /*
>>> -     * Copy the device tree so that it starts immediately after the
>>> image.
>>> -     * After this we will have the U-Boot image and its device tree
>>> ready
>>> -     * for us to start.
>>> -     */
>>> -    debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,
>>> -          fdt_len);
>>> -    src = dst + get_aligned_image_overhead(info, fdt_offset);
>>> -    dst = load_ptr + data_size;
>>> -
>>> -#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
>>> -    board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);
>>> -#endif
>>> -
>>> -    memcpy(dst, src, fdt_len);
>>> +    image_info.load_addr = spl_image->load_addr + spl_image->size;
>>> +    spl_load_fit_image(info, sector, fit, base_offset, node,
>>> &image_info);
>>>          return 0;
>>>    }
>>
>
>
>

[U-Boot] [RFC PATCH 03/11] SPL: FIT: factor out spl_load_fit_image()

[Lokesh Vutla]

On Friday 20 January 2017 07:23 AM, Andre Przywara wrote:
> At the moment we load two images from a FIT image: the actual U-Boot
> image and the DTB. Both times we have very similar code to deal with
> alignment requirement the media we load from imposes upon us.
> Factor out this code into a new function, which we just call twice.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  common/spl/spl_fit.c | 122 +++++++++++++++++++++------------------------------
>  1 file changed, 51 insertions(+), 71 deletions(-)
> 
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 381ed1f..d4149c5 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
>  	return (data_size + info->bl_len - 1) / info->bl_len;
>  }
>  
> +static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
> +			      void *fit, ulong base_offset, int node,
> +			      struct spl_image_info *image_info)
> +{
> +	ulong offset;
> +	size_t length;
> +	ulong load, entry;
> +	void *src;
> +	ulong overhead;
> +	int nr_sectors;
> +
> +	offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
> +	length = fdt_getprop_u32(fit, node, "data-size");
> +	load = fdt_getprop_u32(fit, node, "load");
> +	if (load == -1U && image_info)
> +		load = image_info->load_addr;

What if load_addr is not aligned with ARCH_DMA_MINALIGN like in case of
DT loading (u-boot's load_addr + size cannot be always aligned with
DMA). I keep getting this error when loading DT: "FAT: Misaligned buffer
address (808675a0)."

Something similar is required to fix it:
http://pastebin.ubuntu.com/23850970/

> +	entry = fdt_getprop_u32(fit, node, "entry");
> +
> +	overhead = get_aligned_image_overhead(info, offset);
> +	nr_sectors = get_aligned_image_size(info, overhead + length, offset);
										     ^^^^
Need not add overhead here as get_aligned_image_size() takes care of
adding overhead.

> +
> +	if (info->read(info, sector + get_aligned_image_offset(info, offset),
> +		       nr_sectors, (void*)load) != nr_sectors)
> +		return -EIO;
> +	debug("image: dst=%lx, offset=%lx, size=%lx\n", load, offset,
> +	      (unsigned long)length);
> +
> +	src = (void *)load + overhead;
> +#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
> +	board_fit_image_post_process(&src, &length);
> +#endif
> +
> +	memcpy((void*)load, src, length);
> +
> +	if (image_info) {
> +		image_info->load_addr = load;
> +		image_info->size = length;
> +		image_info->entry_point = entry;

If entry_point is not provided can we default to load_addr?
Existing U-Boot fit image generation does not provide entry option. So
this patch alone breaks boot.(I understand that it is fixed in next
patch but this is breaking git bisectability).

Thanks and regards,
Lokesh


> +	}
> +
> +	return 0;
> +}
> +
>  int spl_load_simple_fit(struct spl_image_info *spl_image,
>  			struct spl_load_info *info, ulong sector, void *fit)
>  {
>  	int sectors;
> -	ulong size, load;
> +	ulong size;
>  	unsigned long count;
> +	struct spl_image_info image_info;
>  	int node, images;
> -	void *load_ptr;
> -	int fdt_offset, fdt_len;
> -	int data_offset, data_size;
>  	int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
> -	int src_sector;
> -	void *dst, *src;
>  
>  	/*
>  	 * Figure out where the external images start. This is the base for the
> @@ -202,82 +241,23 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>  		return -1;
>  	}
>  
> -	/* Get its information and set up the spl_image structure */
> -	data_offset = fdt_getprop_u32(fit, node, "data-offset");
> -	data_size = fdt_getprop_u32(fit, node, "data-size");
> -	load = fdt_getprop_u32(fit, node, "load");
> -	debug("data_offset=%x, data_size=%x\n", data_offset, data_size);
> -	spl_image->load_addr = load;
> -	spl_image->entry_point = load;
> +	/* Load the image and set up the spl_image structure */
> +	spl_load_fit_image(info, sector, fit, base_offset, node, spl_image);
>  	spl_image->os = IH_OS_U_BOOT;
>  
> -	/*
> -	 * Work out where to place the image. We read it so that the first
> -	 * byte will be at 'load'. This may mean we need to load it starting
> -	 * before then, since we can only read whole blocks.
> -	 */
> -	data_offset += base_offset;
> -	sectors = get_aligned_image_size(info, data_size, data_offset);
> -	load_ptr = (void *)load;
> -	debug("U-Boot size %x, data %p\n", data_size, load_ptr);
> -	dst = load_ptr;
> -
> -	/* Read the image */
> -	src_sector = sector + get_aligned_image_offset(info, data_offset);
> -	debug("Aligned image read: dst=%p, src_sector=%x, sectors=%x\n",
> -	      dst, src_sector, sectors);
> -	count = info->read(info, src_sector, sectors, dst);
> -	if (count != sectors)
> -		return -EIO;
> -	debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,
> -	      data_size);
> -	src = dst + get_aligned_image_overhead(info, data_offset);
> -
> -#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
> -	board_fit_image_post_process((void **)&src, (size_t *)&data_size);
> -#endif
> -
> -	memcpy(dst, src, data_size);
> -
>  	/* Figure out which device tree the board wants to use */
>  	node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, 0);
>  	if (node < 0) {
>  		debug("%s: cannot find FDT node\n", __func__);
>  		return node;
>  	}
> -	fdt_offset = fdt_getprop_u32(fit, node, "data-offset");
> -	fdt_len = fdt_getprop_u32(fit, node, "data-size");
>  
>  	/*
> -	 * Read the device tree and place it after the image. There may be
> -	 * some extra data before it since we can only read entire blocks.
> -	 * And also align the destination address to ARCH_DMA_MINALIGN.
> +	 * Read the device tree and place it after the image.
> +	 * Align the destination address to ARCH_DMA_MINALIGN.
>  	 */
> -	dst = (void *)((load + data_size + align_len) & ~align_len);
> -	fdt_offset += base_offset;
> -	sectors = get_aligned_image_size(info, fdt_len, fdt_offset);
> -	src_sector = sector + get_aligned_image_offset(info, fdt_offset);
> -	count = info->read(info, src_sector, sectors, dst);
> -	debug("Aligned fdt read: dst %p, src_sector = %x, sectors %x\n",
> -	      dst, src_sector, sectors);
> -	if (count != sectors)
> -		return -EIO;
> -
> -	/*
> -	 * Copy the device tree so that it starts immediately after the image.
> -	 * After this we will have the U-Boot image and its device tree ready
> -	 * for us to start.
> -	 */
> -	debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,
> -	      fdt_len);
> -	src = dst + get_aligned_image_overhead(info, fdt_offset);
> -	dst = load_ptr + data_size;
> -
> -#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
> -	board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);
> -#endif
> -
> -	memcpy(dst, src, fdt_len);
> +	image_info.load_addr = spl_image->load_addr + spl_image->size;
> +	spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);
>  
>  	return 0;
>  }
> 

[U-Boot] [RFC PATCH 03/11] SPL: FIT: factor out spl_load_fit_image()

[Tom Rini]

On Mon, Jan 23, 2017 at 02:23:40PM +0530, Lokesh Vutla wrote:
> 
> 
> On Friday 20 January 2017 07:23 AM, Andre Przywara wrote:
> > At the moment we load two images from a FIT image: the actual U-Boot
> > image and the DTB. Both times we have very similar code to deal with
> > alignment requirement the media we load from imposes upon us.
> > Factor out this code into a new function, which we just call twice.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  common/spl/spl_fit.c | 122 +++++++++++++++++++++------------------------------
> >  1 file changed, 51 insertions(+), 71 deletions(-)
> > 
> > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> > index 381ed1f..d4149c5 100644
> > --- a/common/spl/spl_fit.c
> > +++ b/common/spl/spl_fit.c
> > @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
> >  	return (data_size + info->bl_len - 1) / info->bl_len;
> >  }
> >  
> > +static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
> > +			      void *fit, ulong base_offset, int node,
> > +			      struct spl_image_info *image_info)
> > +{
> > +	ulong offset;
> > +	size_t length;
> > +	ulong load, entry;
> > +	void *src;
> > +	ulong overhead;
> > +	int nr_sectors;
> > +
> > +	offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
> > +	length = fdt_getprop_u32(fit, node, "data-size");
> > +	load = fdt_getprop_u32(fit, node, "load");
> > +	if (load == -1U && image_info)
> > +		load = image_info->load_addr;
> 
> What if load_addr is not aligned with ARCH_DMA_MINALIGN like in case of
> DT loading (u-boot's load_addr + size cannot be always aligned with
> DMA). I keep getting this error when loading DT: "FAT: Misaligned buffer
> address (808675a0)."

My immediate concern here is that we've found another way we're going to
run into the same old problems of 'large kernel BSS stomps on DT (or
initrd) on ARM32 (arm64 won't because the Image format includes end of
BSS as a field).  If your FDT isn't at base+128MiB (and you have >
128MiB DDR), something is wrong :)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170123/2a129fbf/attachment.sig>

[U-Boot] [RFC PATCH 03/11] SPL: FIT: factor out spl_load_fit_image()

[Lokesh Vutla]

Tom,

On Monday 23 January 2017 06:28 PM, Tom Rini wrote:
> On Mon, Jan 23, 2017 at 02:23:40PM +0530, Lokesh Vutla wrote:
>>
>>
>> On Friday 20 January 2017 07:23 AM, Andre Przywara wrote:
>>> At the moment we load two images from a FIT image: the actual U-Boot
>>> image and the DTB. Both times we have very similar code to deal with
>>> alignment requirement the media we load from imposes upon us.
>>> Factor out this code into a new function, which we just call twice.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  common/spl/spl_fit.c | 122 +++++++++++++++++++++------------------------------
>>>  1 file changed, 51 insertions(+), 71 deletions(-)
>>>
>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>> index 381ed1f..d4149c5 100644
>>> --- a/common/spl/spl_fit.c
>>> +++ b/common/spl/spl_fit.c
>>> @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
>>>  	return (data_size + info->bl_len - 1) / info->bl_len;
>>>  }
>>>  
>>> +static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
>>> +			      void *fit, ulong base_offset, int node,
>>> +			      struct spl_image_info *image_info)
>>> +{
>>> +	ulong offset;
>>> +	size_t length;
>>> +	ulong load, entry;
>>> +	void *src;
>>> +	ulong overhead;
>>> +	int nr_sectors;
>>> +
>>> +	offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
>>> +	length = fdt_getprop_u32(fit, node, "data-size");
>>> +	load = fdt_getprop_u32(fit, node, "load");
>>> +	if (load == -1U && image_info)
>>> +		load = image_info->load_addr;
>>
>> What if load_addr is not aligned with ARCH_DMA_MINALIGN like in case of
>> DT loading (u-boot's load_addr + size cannot be always aligned with
>> DMA). I keep getting this error when loading DT: "FAT: Misaligned buffer
>> address (808675a0)."
> 
> My immediate concern here is that we've found another way we're going to
> run into the same old problems of 'large kernel BSS stomps on DT (or
> initrd) on ARM32 (arm64 won't because the Image format includes end of
> BSS as a field).  If your FDT isn't at base+128MiB (and you have >
> 128MiB DDR), something is wrong :)

I guess the problem here is FDT for u-boot, which is loaded at the end
of u-boot which depends on u-boot's load address and size. I agree that
for kernel the DTB is loaded at base + 128MB. or am I missing something?

Thanks and regards,
Lokesh

[U-Boot] [RFC PATCH 03/11] SPL: FIT: factor out spl_load_fit_image()

[Tom Rini]

On Mon, Jan 23, 2017 at 07:01:38PM +0530, Lokesh Vutla wrote:
> Tom,
> 
> On Monday 23 January 2017 06:28 PM, Tom Rini wrote:
> > On Mon, Jan 23, 2017 at 02:23:40PM +0530, Lokesh Vutla wrote:
> >>
> >>
> >> On Friday 20 January 2017 07:23 AM, Andre Przywara wrote:
> >>> At the moment we load two images from a FIT image: the actual U-Boot
> >>> image and the DTB. Both times we have very similar code to deal with
> >>> alignment requirement the media we load from imposes upon us.
> >>> Factor out this code into a new function, which we just call twice.
> >>>
> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >>> ---
> >>>  common/spl/spl_fit.c | 122 +++++++++++++++++++++------------------------------
> >>>  1 file changed, 51 insertions(+), 71 deletions(-)
> >>>
> >>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> >>> index 381ed1f..d4149c5 100644
> >>> --- a/common/spl/spl_fit.c
> >>> +++ b/common/spl/spl_fit.c
> >>> @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
> >>>  	return (data_size + info->bl_len - 1) / info->bl_len;
> >>>  }
> >>>  
> >>> +static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
> >>> +			      void *fit, ulong base_offset, int node,
> >>> +			      struct spl_image_info *image_info)
> >>> +{
> >>> +	ulong offset;
> >>> +	size_t length;
> >>> +	ulong load, entry;
> >>> +	void *src;
> >>> +	ulong overhead;
> >>> +	int nr_sectors;
> >>> +
> >>> +	offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
> >>> +	length = fdt_getprop_u32(fit, node, "data-size");
> >>> +	load = fdt_getprop_u32(fit, node, "load");
> >>> +	if (load == -1U && image_info)
> >>> +		load = image_info->load_addr;
> >>
> >> What if load_addr is not aligned with ARCH_DMA_MINALIGN like in case of
> >> DT loading (u-boot's load_addr + size cannot be always aligned with
> >> DMA). I keep getting this error when loading DT: "FAT: Misaligned buffer
> >> address (808675a0)."
> > 
> > My immediate concern here is that we've found another way we're going to
> > run into the same old problems of 'large kernel BSS stomps on DT (or
> > initrd) on ARM32 (arm64 won't because the Image format includes end of
> > BSS as a field).  If your FDT isn't at base+128MiB (and you have >
> > 128MiB DDR), something is wrong :)
> 
> I guess the problem here is FDT for u-boot, which is loaded at the end
> of u-boot which depends on u-boot's load address and size. I agree that
> for kernel the DTB is loaded at base + 128MB. or am I missing something?

Ah, sorry. misread, nevermind.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170123/807dca26/attachment.sig>

[U-Boot] [RFC PATCH 03/11] SPL: FIT: factor out spl_load_fit_image()

[André Przywara]

On 23/01/17 08:53, Lokesh Vutla wrote:

Hi Lokesh,

thanks a lot for having a thorough look at it!

>> On Friday 20 January 2017 07:23 AM, Andre Przywara wrote:
>> At the moment we load two images from a FIT image: the actual U-Boot
>> image and the DTB. Both times we have very similar code to deal with
>> alignment requirement the media we load from imposes upon us.
>> Factor out this code into a new function, which we just call twice.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  common/spl/spl_fit.c | 122 +++++++++++++++++++++------------------------------
>>  1 file changed, 51 insertions(+), 71 deletions(-)
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index 381ed1f..d4149c5 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -138,19 +138,58 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
>>  	return (data_size + info->bl_len - 1) / info->bl_len;
>>  }
>>  
>> +static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
>> +			      void *fit, ulong base_offset, int node,
>> +			      struct spl_image_info *image_info)
>> +{
>> +	ulong offset;
>> +	size_t length;
>> +	ulong load, entry;
>> +	void *src;
>> +	ulong overhead;
>> +	int nr_sectors;
>> +
>> +	offset = fdt_getprop_u32(fit, node, "data-offset") + base_offset;
>> +	length = fdt_getprop_u32(fit, node, "data-size");
>> +	load = fdt_getprop_u32(fit, node, "load");
>> +	if (load == -1U && image_info)
>> +		load = image_info->load_addr;
> 
> What if load_addr is not aligned with ARCH_DMA_MINALIGN like in case of
> DT loading (u-boot's load_addr + size cannot be always aligned with
> DMA). I keep getting this error when loading DT: "FAT: Misaligned buffer
> address (808675a0)."

Ah, good point, though I didn't encounter this error.

> Something similar is required to fix it:
> http://pastebin.ubuntu.com/23850970/

Yeah, looks like it. I try to bake this in an upcoming release.

>> +	entry = fdt_getprop_u32(fit, node, "entry");
>> +
>> +	overhead = get_aligned_image_overhead(info, offset);
>> +	nr_sectors = get_aligned_image_size(info, overhead + length, offset);
> 										     ^^^^
> Need not add overhead here as get_aligned_image_size() takes care of
> adding overhead.

Right, I somehow got lost in translation here, probably during some
rebasing/refactoring.

>> +
>> +	if (info->read(info, sector + get_aligned_image_offset(info, offset),
>> +		       nr_sectors, (void*)load) != nr_sectors)
>> +		return -EIO;
>> +	debug("image: dst=%lx, offset=%lx, size=%lx\n", load, offset,
>> +	      (unsigned long)length);
>> +
>> +	src = (void *)load + overhead;
>> +#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
>> +	board_fit_image_post_process(&src, &length);
>> +#endif
>> +
>> +	memcpy((void*)load, src, length);
>> +
>> +	if (image_info) {
>> +		image_info->load_addr = load;
>> +		image_info->size = length;
>> +		image_info->entry_point = entry;
> 
> If entry_point is not provided can we default to load_addr?
> Existing U-Boot fit image generation does not provide entry option. So
> this patch alone breaks boot.(I understand that it is fixed in next
> patch but this is breaking git bisectability).

Good point, this is indeed a good idea.

Cheers,
Andre.

> Thanks and regards,
> Lokesh
> 
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  int spl_load_simple_fit(struct spl_image_info *spl_image,
>>  			struct spl_load_info *info, ulong sector, void *fit)
>>  {
>>  	int sectors;
>> -	ulong size, load;
>> +	ulong size;
>>  	unsigned long count;
>> +	struct spl_image_info image_info;
>>  	int node, images;
>> -	void *load_ptr;
>> -	int fdt_offset, fdt_len;
>> -	int data_offset, data_size;
>>  	int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
>> -	int src_sector;
>> -	void *dst, *src;
>>  
>>  	/*
>>  	 * Figure out where the external images start. This is the base for the
>> @@ -202,82 +241,23 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>>  		return -1;
>>  	}
>>  
>> -	/* Get its information and set up the spl_image structure */
>> -	data_offset = fdt_getprop_u32(fit, node, "data-offset");
>> -	data_size = fdt_getprop_u32(fit, node, "data-size");
>> -	load = fdt_getprop_u32(fit, node, "load");
>> -	debug("data_offset=%x, data_size=%x\n", data_offset, data_size);
>> -	spl_image->load_addr = load;
>> -	spl_image->entry_point = load;
>> +	/* Load the image and set up the spl_image structure */
>> +	spl_load_fit_image(info, sector, fit, base_offset, node, spl_image);
>>  	spl_image->os = IH_OS_U_BOOT;
>>  
>> -	/*
>> -	 * Work out where to place the image. We read it so that the first
>> -	 * byte will be at 'load'. This may mean we need to load it starting
>> -	 * before then, since we can only read whole blocks.
>> -	 */
>> -	data_offset += base_offset;
>> -	sectors = get_aligned_image_size(info, data_size, data_offset);
>> -	load_ptr = (void *)load;
>> -	debug("U-Boot size %x, data %p\n", data_size, load_ptr);
>> -	dst = load_ptr;
>> -
>> -	/* Read the image */
>> -	src_sector = sector + get_aligned_image_offset(info, data_offset);
>> -	debug("Aligned image read: dst=%p, src_sector=%x, sectors=%x\n",
>> -	      dst, src_sector, sectors);
>> -	count = info->read(info, src_sector, sectors, dst);
>> -	if (count != sectors)
>> -		return -EIO;
>> -	debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,
>> -	      data_size);
>> -	src = dst + get_aligned_image_overhead(info, data_offset);
>> -
>> -#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
>> -	board_fit_image_post_process((void **)&src, (size_t *)&data_size);
>> -#endif
>> -
>> -	memcpy(dst, src, data_size);
>> -
>>  	/* Figure out which device tree the board wants to use */
>>  	node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, 0);
>>  	if (node < 0) {
>>  		debug("%s: cannot find FDT node\n", __func__);
>>  		return node;
>>  	}
>> -	fdt_offset = fdt_getprop_u32(fit, node, "data-offset");
>> -	fdt_len = fdt_getprop_u32(fit, node, "data-size");
>>  
>>  	/*
>> -	 * Read the device tree and place it after the image. There may be
>> -	 * some extra data before it since we can only read entire blocks.
>> -	 * And also align the destination address to ARCH_DMA_MINALIGN.
>> +	 * Read the device tree and place it after the image.
>> +	 * Align the destination address to ARCH_DMA_MINALIGN.
>>  	 */
>> -	dst = (void *)((load + data_size + align_len) & ~align_len);
>> -	fdt_offset += base_offset;
>> -	sectors = get_aligned_image_size(info, fdt_len, fdt_offset);
>> -	src_sector = sector + get_aligned_image_offset(info, fdt_offset);
>> -	count = info->read(info, src_sector, sectors, dst);
>> -	debug("Aligned fdt read: dst %p, src_sector = %x, sectors %x\n",
>> -	      dst, src_sector, sectors);
>> -	if (count != sectors)
>> -		return -EIO;
>> -
>> -	/*
>> -	 * Copy the device tree so that it starts immediately after the image.
>> -	 * After this we will have the U-Boot image and its device tree ready
>> -	 * for us to start.
>> -	 */
>> -	debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,
>> -	      fdt_len);
>> -	src = dst + get_aligned_image_overhead(info, fdt_offset);
>> -	dst = load_ptr + data_size;
>> -
>> -#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS
>> -	board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);
>> -#endif
>> -
>> -	memcpy(dst, src, fdt_len);
>> +	image_info.load_addr = spl_image->load_addr + spl_image->size;
>> +	spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);
>>  
>>  	return 0;
>>  }
>>

[U-Boot] [RFC PATCH 04/11] SPL: FIT: allow loading multiple images

[Andre Przywara]

So far we were not using the FIT image format to its full potential:
The SPL FIT loader was just loading the first image from the /images
node plus one of the listed DTBs.
Now with the refactored loader code it's easy to load an arbitrary
number of images in addition to the two mentioned above.
As described in the FIT image source file format description, iterate
over all images listed at the "loadables" property in the configuration
node and load every image at its desired location.
This allows to load any kind of images:
- firmware images to execute before U-Boot proper (for instance
  ARM Trusted Firmware (ATF))
- firmware images for management processors (SCP, arisc, ...)
- firmware images for devices like WiFi controllers
- bit files for FPGAs
- additional configuration data
- kernels and/or ramdisks
The actual usage of this feature would be platform and/or board specific.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 common/spl/spl_fit.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index d4149c5..18269f7 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -190,6 +190,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	struct spl_image_info image_info;
 	int node, images;
 	int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
+	int index = 0;
 
 	/*
 	 * Figure out where the external images start. This is the base for the
@@ -234,6 +235,11 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	if (node < 0) {
 		debug("could not find firmware image, trying loadables...\n");
 		node = spl_fit_get_image_node(fit, images, "loadables", 0);
+		/*
+		 * If we pick the U-Boot image from "loadables", start at
+		 * the second image when later loading additional images.
+		 */
+		index = 1;
 	}
 	if (node < 0) {
 		debug("%s: Cannot find u-boot image node: %d\n",
@@ -259,5 +265,26 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	image_info.load_addr = spl_image->load_addr + spl_image->size;
 	spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);
 
+	/* Now check if there are more images for us to load */
+	for (; ; index++) {
+		node = spl_fit_get_image_node(fit, images, "loadables", index);
+		if (node < 0)
+			break;
+
+		spl_load_fit_image(info, sector, fit, base_offset, node,
+				   &image_info);
+
+		/*
+		 * If the "firmware" image did not provide an entry point,
+		 * use the first valid entry point from the loadables.
+		 */
+		if (spl_image->entry_point == -1U &&
+		    image_info.entry_point != -1U)
+			spl_image->entry_point = image_info.entry_point;
+	}
+
+	if (spl_image->entry_point == -1U || spl_image->entry_point == 0)
+		spl_image->entry_point = spl_image->load_addr;
+
 	return 0;
 }
-- 
2.8.2

[U-Boot] [RFC PATCH 04/11] SPL: FIT: allow loading multiple images

[Kever Yang]

Hi Andre,

     Thanks for your patches, this is great help for enable ATF on 
U-Boot SPL.
For ATF use case, we would like to identify which one is bl31 for we need to
get entry point for it while we only need load address for other image.
Any idea on get this information from different "loadables"?

Thanks,
- Kever
On 01/20/2017 09:53 AM, Andre Przywara wrote:
> So far we were not using the FIT image format to its full potential:
> The SPL FIT loader was just loading the first image from the /images
> node plus one of the listed DTBs.
> Now with the refactored loader code it's easy to load an arbitrary
> number of images in addition to the two mentioned above.
> As described in the FIT image source file format description, iterate
> over all images listed at the "loadables" property in the configuration
> node and load every image at its desired location.
> This allows to load any kind of images:
> - firmware images to execute before U-Boot proper (for instance
>    ARM Trusted Firmware (ATF))
> - firmware images for management processors (SCP, arisc, ...)
> - firmware images for devices like WiFi controllers
> - bit files for FPGAs
> - additional configuration data
> - kernels and/or ramdisks
> The actual usage of this feature would be platform and/or board specific.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>   common/spl/spl_fit.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index d4149c5..18269f7 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -190,6 +190,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>   	struct spl_image_info image_info;
>   	int node, images;
>   	int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
> +	int index = 0;
>   
>   	/*
>   	 * Figure out where the external images start. This is the base for the
> @@ -234,6 +235,11 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>   	if (node < 0) {
>   		debug("could not find firmware image, trying loadables...\n");
>   		node = spl_fit_get_image_node(fit, images, "loadables", 0);
> +		/*
> +		 * If we pick the U-Boot image from "loadables", start at
> +		 * the second image when later loading additional images.
> +		 */
> +		index = 1;
>   	}
>   	if (node < 0) {
>   		debug("%s: Cannot find u-boot image node: %d\n",
> @@ -259,5 +265,26 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>   	image_info.load_addr = spl_image->load_addr + spl_image->size;
>   	spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);
>   
> +	/* Now check if there are more images for us to load */
> +	for (; ; index++) {
> +		node = spl_fit_get_image_node(fit, images, "loadables", index);
> +		if (node < 0)
> +			break;
> +
> +		spl_load_fit_image(info, sector, fit, base_offset, node,
> +				   &image_info);
> +
> +		/*
> +		 * If the "firmware" image did not provide an entry point,
> +		 * use the first valid entry point from the loadables.
> +		 */
> +		if (spl_image->entry_point == -1U &&
> +		    image_info.entry_point != -1U)
> +			spl_image->entry_point = image_info.entry_point;
> +	}
> +
> +	if (spl_image->entry_point == -1U || spl_image->entry_point == 0)
> +		spl_image->entry_point = spl_image->load_addr;
> +
>   	return 0;
>   }

[U-Boot] [RFC PATCH 04/11] SPL: FIT: allow loading multiple images

[André Przywara]

On 22/01/17 07:08, Kever Yang wrote:
> Hi Andre,
> 
>     Thanks for your patches, this is great help for enable ATF on U-Boot
> SPL.
> For ATF use case, we would like to identify which one is bl31 for we
> need to
> get entry point for it while we only need load address for other image.
> Any idea on get this information from different "loadables"?

So I thought this use case is covered by the current scheme?

See below ...

> On 01/20/2017 09:53 AM, Andre Przywara wrote:
>> So far we were not using the FIT image format to its full potential:
>> The SPL FIT loader was just loading the first image from the /images
>> node plus one of the listed DTBs.
>> Now with the refactored loader code it's easy to load an arbitrary
>> number of images in addition to the two mentioned above.
>> As described in the FIT image source file format description, iterate
>> over all images listed at the "loadables" property in the configuration
>> node and load every image at its desired location.
>> This allows to load any kind of images:
>> - firmware images to execute before U-Boot proper (for instance
>>    ARM Trusted Firmware (ATF))
>> - firmware images for management processors (SCP, arisc, ...)
>> - firmware images for devices like WiFi controllers
>> - bit files for FPGAs
>> - additional configuration data
>> - kernels and/or ramdisks
>> The actual usage of this feature would be platform and/or board specific.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>   common/spl/spl_fit.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index d4149c5..18269f7 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -190,6 +190,7 @@ int spl_load_simple_fit(struct spl_image_info
>> *spl_image,
>>       struct spl_image_info image_info;
>>       int node, images;
>>       int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
>> +    int index = 0;
>>         /*
>>        * Figure out where the external images start. This is the base
>> for the
>> @@ -234,6 +235,11 @@ int spl_load_simple_fit(struct spl_image_info
>> *spl_image,
>>       if (node < 0) {
>>           debug("could not find firmware image, trying loadables...\n");
>>           node = spl_fit_get_image_node(fit, images, "loadables", 0);
>> +        /*
>> +         * If we pick the U-Boot image from "loadables", start at
>> +         * the second image when later loading additional images.
>> +         */
>> +        index = 1;
>>       }
>>       if (node < 0) {
>>           debug("%s: Cannot find u-boot image node: %d\n",
>> @@ -259,5 +265,26 @@ int spl_load_simple_fit(struct spl_image_info
>> *spl_image,
>>       image_info.load_addr = spl_image->load_addr + spl_image->size;
>>       spl_load_fit_image(info, sector, fit, base_offset, node,
>> &image_info);
>>   +    /* Now check if there are more images for us to load */
>> +    for (; ; index++) {
>> +        node = spl_fit_get_image_node(fit, images, "loadables", index);
>> +        if (node < 0)
>> +            break;
>> +
>> +        spl_load_fit_image(info, sector, fit, base_offset, node,
>> +                   &image_info);
>> +
>> +        /*
>> +         * If the "firmware" image did not provide an entry point,
>> +         * use the first valid entry point from the loadables.
>> +         */
>> +        if (spl_image->entry_point == -1U &&
>> +            image_info.entry_point != -1U)
>> +            spl_image->entry_point = image_info.entry_point;

So this part here saves the entry point from the first image which
provides an "entry" property in the loadables section, given that
"firmware" didn't provide (a legal) one.

So depending on what you want, these are the options:
a) You want to branch to U-Boot (default case if mkimage -f auto is
used): You either don't specify an explicit entry point at all or
provide an "entry" property for the U-Boot "firmware" image.
The code below at the end of this function takes care of this.
b) You want to branch to something else (bl31.bin): You make sure there
is no explicit entry point in "firmware", instead put one "entry"
property in the bl31 image description you want to boot. This will then
branch to that instead of U-Boot.
Check the example .its in patch 10/11 for an example of this.

So does this fit for you? Or is there a problem with this?

>> +    }
>> +
>> +    if (spl_image->entry_point == -1U || spl_image->entry_point == 0)
>> +        spl_image->entry_point = spl_image->load_addr;

This statement here takes the load address of the "firmware" image as an
entry point if none is provided explicitly.

Cheers,
Andre.

>> +
>>       return 0;
>>   }

[U-Boot] [RFC PATCH 04/11] SPL: FIT: allow loading multiple images

[Kever Yang]

Hi Andre,

On 01/22/2017 06:58 PM, Andr? Przywara wrote:
> On 22/01/17 07:08, Kever Yang wrote:
>> Hi Andre,
>>
>>      Thanks for your patches, this is great help for enable ATF on U-Boot
>> SPL.
>> For ATF use case, we would like to identify which one is bl31 for we
>> need to
>> get entry point for it while we only need load address for other image.
>> Any idea on get this information from different "loadables"?
> So I thought this use case is covered by the current scheme?
>
> See below ...
>
>> On 01/20/2017 09:53 AM, Andre Przywara wrote:
>>> So far we were not using the FIT image format to its full potential:
>>> The SPL FIT loader was just loading the first image from the /images
>>> node plus one of the listed DTBs.
>>> Now with the refactored loader code it's easy to load an arbitrary
>>> number of images in addition to the two mentioned above.
>>> As described in the FIT image source file format description, iterate
>>> over all images listed at the "loadables" property in the configuration
>>> node and load every image at its desired location.
>>> This allows to load any kind of images:
>>> - firmware images to execute before U-Boot proper (for instance
>>>     ARM Trusted Firmware (ATF))
>>> - firmware images for management processors (SCP, arisc, ...)
>>> - firmware images for devices like WiFi controllers
>>> - bit files for FPGAs
>>> - additional configuration data
>>> - kernels and/or ramdisks
>>> The actual usage of this feature would be platform and/or board specific.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>    common/spl/spl_fit.c | 27 +++++++++++++++++++++++++++
>>>    1 file changed, 27 insertions(+)
>>>
>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>> index d4149c5..18269f7 100644
>>> --- a/common/spl/spl_fit.c
>>> +++ b/common/spl/spl_fit.c
>>> @@ -190,6 +190,7 @@ int spl_load_simple_fit(struct spl_image_info
>>> *spl_image,
>>>        struct spl_image_info image_info;
>>>        int node, images;
>>>        int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
>>> +    int index = 0;
>>>          /*
>>>         * Figure out where the external images start. This is the base
>>> for the
>>> @@ -234,6 +235,11 @@ int spl_load_simple_fit(struct spl_image_info
>>> *spl_image,
>>>        if (node < 0) {
>>>            debug("could not find firmware image, trying loadables...\n");
>>>            node = spl_fit_get_image_node(fit, images, "loadables", 0);
>>> +        /*
>>> +         * If we pick the U-Boot image from "loadables", start at
>>> +         * the second image when later loading additional images.
>>> +         */
>>> +        index = 1;
>>>        }
>>>        if (node < 0) {
>>>            debug("%s: Cannot find u-boot image node: %d\n",
>>> @@ -259,5 +265,26 @@ int spl_load_simple_fit(struct spl_image_info
>>> *spl_image,
>>>        image_info.load_addr = spl_image->load_addr + spl_image->size;
>>>        spl_load_fit_image(info, sector, fit, base_offset, node,
>>> &image_info);
>>>    +    /* Now check if there are more images for us to load */
>>> +    for (; ; index++) {
>>> +        node = spl_fit_get_image_node(fit, images, "loadables", index);
>>> +        if (node < 0)
>>> +            break;
>>> +
>>> +        spl_load_fit_image(info, sector, fit, base_offset, node,
>>> +                   &image_info);
>>> +
>>> +        /*
>>> +         * If the "firmware" image did not provide an entry point,
>>> +         * use the first valid entry point from the loadables.
>>> +         */
>>> +        if (spl_image->entry_point == -1U &&
>>> +            image_info.entry_point != -1U)
>>> +            spl_image->entry_point = image_info.entry_point;
> So this part here saves the entry point from the first image which
> provides an "entry" property in the loadables section, given that
> "firmware" didn't provide (a legal) one.
>
> So depending on what you want, these are the options:
> a) You want to branch to U-Boot (default case if mkimage -f auto is
> used): You either don't specify an explicit entry point at all or
> provide an "entry" property for the U-Boot "firmware" image.
> The code below at the end of this function takes care of this.
> b) You want to branch to something else (bl31.bin): You make sure there
> is no explicit entry point in "firmware", instead put one "entry"
> property in the bl31 image description you want to boot. This will then
> branch to that instead of U-Boot.
> Check the example .its in patch 10/11 for an example of this.
>
> So does this fit for you? Or is there a problem with this?

So that means only one 'entry' can be present even if there are more 
than one image, right?
I think every "firmware" image is possible to have both "load" and 
"entry" node,
for example if we have bl31, bl32 and bl33(U-Boot) in one FIT image,
then we need to fill the bl31_params_t structure with bl32 entry point 
and U-Boot entry point,
so that bl31 able to call bl32 and bl33.
That's why I think we need to identify each of the "firmware", but not 
only know they are
"loadables".

Thanks,
- Kever

>>> +    }
>>> +
>>> +    if (spl_image->entry_point == -1U || spl_image->entry_point == 0)
>>> +        spl_image->entry_point = spl_image->load_addr;
> This statement here takes the load address of the "firmware" image as an
> entry point if none is provided explicitly.
>
> Cheers,
> Andre.
>
>>> +
>>>        return 0;
>>>    }
>
>
>
>
>

[U-Boot] [RFC PATCH 04/11] SPL: FIT: allow loading multiple images

[Michal Simek]

On 23.1.2017 03:49, Kever Yang wrote:
> Hi Andre,
> 
> On 01/22/2017 06:58 PM, Andr? Przywara wrote:
>> On 22/01/17 07:08, Kever Yang wrote:
>>> Hi Andre,
>>>
>>>      Thanks for your patches, this is great help for enable ATF on
>>> U-Boot
>>> SPL.
>>> For ATF use case, we would like to identify which one is bl31 for we
>>> need to
>>> get entry point for it while we only need load address for other image.
>>> Any idea on get this information from different "loadables"?
>> So I thought this use case is covered by the current scheme?
>>
>> See below ...
>>
>>> On 01/20/2017 09:53 AM, Andre Przywara wrote:
>>>> So far we were not using the FIT image format to its full potential:
>>>> The SPL FIT loader was just loading the first image from the /images
>>>> node plus one of the listed DTBs.
>>>> Now with the refactored loader code it's easy to load an arbitrary
>>>> number of images in addition to the two mentioned above.
>>>> As described in the FIT image source file format description, iterate
>>>> over all images listed at the "loadables" property in the configuration
>>>> node and load every image at its desired location.
>>>> This allows to load any kind of images:
>>>> - firmware images to execute before U-Boot proper (for instance
>>>>     ARM Trusted Firmware (ATF))
>>>> - firmware images for management processors (SCP, arisc, ...)
>>>> - firmware images for devices like WiFi controllers
>>>> - bit files for FPGAs
>>>> - additional configuration data
>>>> - kernels and/or ramdisks
>>>> The actual usage of this feature would be platform and/or board
>>>> specific.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>>    common/spl/spl_fit.c | 27 +++++++++++++++++++++++++++
>>>>    1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>>> index d4149c5..18269f7 100644
>>>> --- a/common/spl/spl_fit.c
>>>> +++ b/common/spl/spl_fit.c
>>>> @@ -190,6 +190,7 @@ int spl_load_simple_fit(struct spl_image_info
>>>> *spl_image,
>>>>        struct spl_image_info image_info;
>>>>        int node, images;
>>>>        int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
>>>> +    int index = 0;
>>>>          /*
>>>>         * Figure out where the external images start. This is the base
>>>> for the
>>>> @@ -234,6 +235,11 @@ int spl_load_simple_fit(struct spl_image_info
>>>> *spl_image,
>>>>        if (node < 0) {
>>>>            debug("could not find firmware image, trying
>>>> loadables...\n");
>>>>            node = spl_fit_get_image_node(fit, images, "loadables", 0);
>>>> +        /*
>>>> +         * If we pick the U-Boot image from "loadables", start at
>>>> +         * the second image when later loading additional images.
>>>> +         */
>>>> +        index = 1;
>>>>        }
>>>>        if (node < 0) {
>>>>            debug("%s: Cannot find u-boot image node: %d\n",
>>>> @@ -259,5 +265,26 @@ int spl_load_simple_fit(struct spl_image_info
>>>> *spl_image,
>>>>        image_info.load_addr = spl_image->load_addr + spl_image->size;
>>>>        spl_load_fit_image(info, sector, fit, base_offset, node,
>>>> &image_info);
>>>>    +    /* Now check if there are more images for us to load */
>>>> +    for (; ; index++) {
>>>> +        node = spl_fit_get_image_node(fit, images, "loadables",
>>>> index);
>>>> +        if (node < 0)
>>>> +            break;
>>>> +
>>>> +        spl_load_fit_image(info, sector, fit, base_offset, node,
>>>> +                   &image_info);
>>>> +
>>>> +        /*
>>>> +         * If the "firmware" image did not provide an entry point,
>>>> +         * use the first valid entry point from the loadables.
>>>> +         */
>>>> +        if (spl_image->entry_point == -1U &&
>>>> +            image_info.entry_point != -1U)
>>>> +            spl_image->entry_point = image_info.entry_point;
>> So this part here saves the entry point from the first image which
>> provides an "entry" property in the loadables section, given that
>> "firmware" didn't provide (a legal) one.
>>
>> So depending on what you want, these are the options:
>> a) You want to branch to U-Boot (default case if mkimage -f auto is
>> used): You either don't specify an explicit entry point at all or
>> provide an "entry" property for the U-Boot "firmware" image.
>> The code below at the end of this function takes care of this.
>> b) You want to branch to something else (bl31.bin): You make sure there
>> is no explicit entry point in "firmware", instead put one "entry"
>> property in the bl31 image description you want to boot. This will then
>> branch to that instead of U-Boot.
>> Check the example .its in patch 10/11 for an example of this.
>>
>> So does this fit for you? Or is there a problem with this?
> 
> So that means only one 'entry' can be present even if there are more
> than one image, right?
> I think every "firmware" image is possible to have both "load" and
> "entry" node,
> for example if we have bl31, bl32 and bl33(U-Boot) in one FIT image,
> then we need to fill the bl31_params_t structure with bl32 entry point
> and U-Boot entry point,
> so that bl31 able to call bl32 and bl33.
> That's why I think we need to identify each of the "firmware", but not
> only know they are
> "loadables".

You need put this information somewhere. It means node itself should
contain that information. And there is no field for it. It means if you
want support it you need to extend it. Definitely for "full" boot you
would need it.

Thanks,
Michal

[U-Boot] [RFC PATCH 04/11] SPL: FIT: allow loading multiple images

[André Przywara]

On 23/01/17 02:49, Kever Yang wrote:
> Hi Andre,
> 
> On 01/22/2017 06:58 PM, Andr? Przywara wrote:
>> On 22/01/17 07:08, Kever Yang wrote:
>>> Hi Andre,
>>>
>>>      Thanks for your patches, this is great help for enable ATF on
>>> U-Boot
>>> SPL.
>>> For ATF use case, we would like to identify which one is bl31 for we
>>> need to
>>> get entry point for it while we only need load address for other image.
>>> Any idea on get this information from different "loadables"?
>> So I thought this use case is covered by the current scheme?
>>
>> See below ...
>>
>>> On 01/20/2017 09:53 AM, Andre Przywara wrote:
>>>> So far we were not using the FIT image format to its full potential:
>>>> The SPL FIT loader was just loading the first image from the /images
>>>> node plus one of the listed DTBs.
>>>> Now with the refactored loader code it's easy to load an arbitrary
>>>> number of images in addition to the two mentioned above.
>>>> As described in the FIT image source file format description, iterate
>>>> over all images listed at the "loadables" property in the configuration
>>>> node and load every image at its desired location.
>>>> This allows to load any kind of images:
>>>> - firmware images to execute before U-Boot proper (for instance
>>>>     ARM Trusted Firmware (ATF))
>>>> - firmware images for management processors (SCP, arisc, ...)
>>>> - firmware images for devices like WiFi controllers
>>>> - bit files for FPGAs
>>>> - additional configuration data
>>>> - kernels and/or ramdisks
>>>> The actual usage of this feature would be platform and/or board
>>>> specific.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>>    common/spl/spl_fit.c | 27 +++++++++++++++++++++++++++
>>>>    1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>>> index d4149c5..18269f7 100644
>>>> --- a/common/spl/spl_fit.c
>>>> +++ b/common/spl/spl_fit.c
>>>> @@ -190,6 +190,7 @@ int spl_load_simple_fit(struct spl_image_info
>>>> *spl_image,
>>>>        struct spl_image_info image_info;
>>>>        int node, images;
>>>>        int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
>>>> +    int index = 0;
>>>>          /*
>>>>         * Figure out where the external images start. This is the base
>>>> for the
>>>> @@ -234,6 +235,11 @@ int spl_load_simple_fit(struct spl_image_info
>>>> *spl_image,
>>>>        if (node < 0) {
>>>>            debug("could not find firmware image, trying
>>>> loadables...\n");
>>>>            node = spl_fit_get_image_node(fit, images, "loadables", 0);
>>>> +        /*
>>>> +         * If we pick the U-Boot image from "loadables", start at
>>>> +         * the second image when later loading additional images.
>>>> +         */
>>>> +        index = 1;
>>>>        }
>>>>        if (node < 0) {
>>>>            debug("%s: Cannot find u-boot image node: %d\n",
>>>> @@ -259,5 +265,26 @@ int spl_load_simple_fit(struct spl_image_info
>>>> *spl_image,
>>>>        image_info.load_addr = spl_image->load_addr + spl_image->size;
>>>>        spl_load_fit_image(info, sector, fit, base_offset, node,
>>>> &image_info);
>>>>    +    /* Now check if there are more images for us to load */
>>>> +    for (; ; index++) {
>>>> +        node = spl_fit_get_image_node(fit, images, "loadables",
>>>> index);
>>>> +        if (node < 0)
>>>> +            break;
>>>> +
>>>> +        spl_load_fit_image(info, sector, fit, base_offset, node,
>>>> +                   &image_info);
>>>> +
>>>> +        /*
>>>> +         * If the "firmware" image did not provide an entry point,
>>>> +         * use the first valid entry point from the loadables.
>>>> +         */
>>>> +        if (spl_image->entry_point == -1U &&
>>>> +            image_info.entry_point != -1U)
>>>> +            spl_image->entry_point = image_info.entry_point;
>> So this part here saves the entry point from the first image which
>> provides an "entry" property in the loadables section, given that
>> "firmware" didn't provide (a legal) one.
>>
>> So depending on what you want, these are the options:
>> a) You want to branch to U-Boot (default case if mkimage -f auto is
>> used): You either don't specify an explicit entry point at all or
>> provide an "entry" property for the U-Boot "firmware" image.
>> The code below at the end of this function takes care of this.
>> b) You want to branch to something else (bl31.bin): You make sure there
>> is no explicit entry point in "firmware", instead put one "entry"
>> property in the bl31 image description you want to boot. This will then
>> branch to that instead of U-Boot.
>> Check the example .its in patch 10/11 for an example of this.
>>
>> So does this fit for you? Or is there a problem with this?
> 
> So that means only one 'entry' can be present even if there are more
> than one image, right?

Well, there can be more nodes with entry properties, but the code just
takes the first one and ignores the others. Because logically there is
only one entry point after the SPL.

> I think every "firmware" image is possible to have both "load" and
> "entry" node,
> for example if we have bl31, bl32 and bl33(U-Boot) in one FIT image,
> then we need to fill the bl31_params_t structure with bl32 entry point
> and U-Boot entry point,
> so that bl31 able to call bl32 and bl33.

Well, yes, but this is very ATF (or platform) specific. This series aims
to use a generic solution, which does not need to be adapted to every
platform. At the end of the day U-Boot can only jump once into an image.

I guess we could implement something which waits for a return: "bx"
instead of "b", upon return try the next images. If it doesn't return:
fair enough, it probably just went ahead with booting on its own. But I
am not sure this is overly useful, since any kind of firmware would
probably want to setup the system and thus would destroy U-Boot's setup
(page tables, stack pointer, etc.) And I think it wouldn't help in your
case.

I think what we should do instead is allowing platform specific
additions to the loader code. For instance I saw that x86 introduced a
"setup" property to hook in some pre-boot code. This is used by U-Boot
proper, not SPL, but we could piggy-back on this idea. Maybe a "setup"
property in a configuration node points to some special image node,
which contains platform specific properties. Each platform is then
expected to provide a specialised node parser, which can then deal with
this setup node. Along the lines of:

images {
	atf-setup at 1 {
		data = /incbin/("../../bl31.bin");
		atf-bl32 = "bl32 at 1";
		atf-bl33 = "uboot at 1";
		atf-bl31-params-offset = <0x124>;
		load = < ... >;
		entry = < ... >;
	};
	bl32 at 1 {
		data = /incbin/("../../bl32.bin");
		load = <...>;
	};
	uboot at 1 {
		data = /incbin/("../../u-boot-nodtb.bin");
		load = <...>;
	};
...
};
configurations {
	config at 1 {
		description = "rk3399-evb";
		firmware = "uboot at 1";
		setup = "atf-setup at 1";
		loadables = "bl32 at 1";
		fdt = "fdt at 1";
	};
};

Then have some RK specific parser code that follows the "setup" property
(in this case to "atf-setup at 1") and reads the special properties in
there to do its magic: read the load addresses of the referenced images
and setup the structure, its address being in "atf-bl31-params-offset".

This way you could control everything from the FIT image nodes. Ideally
this would be even RK agnostic, instead providing a generic ATF solution.

Another possibility would be to have some small shim that gets loaded
instead of bl31.bin and which fills up the bl31_params_t structure.
Though I guess it would be hard to communicate the parameters.

Or can't you simply hard code the bl32 and bl33 load addresses into
bl31? I can imagine you end up with them being loaded on certain
addresses (SRAM) only anyway. On Allwinner for instance the U-Boot load
address is hardcoded in bl31.bin, so we don't need to take care of this.

> That's why I think we need to identify each of the "firmware", but not
> only know they are "loadables".

I don't know if people like the idea of introducing a lot of special
properties into a configuration node, that's why the idea of confining
everything into a platform-specific node. Maybe we even use compatible=
strings?

Cheers,
Andre.

>>>> +    }
>>>> +
>>>> +    if (spl_image->entry_point == -1U || spl_image->entry_point == 0)
>>>> +        spl_image->entry_point = spl_image->load_addr;
>> This statement here takes the load address of the "firmware" image as an
>> entry point if none is provided explicitly.
>>
>> Cheers,
>> Andre.
>>
>>>> +
>>>>        return 0;
>>>>    }
>>
>>
>>
>>
>>
> 
> 

[U-Boot] [RFC PATCH 04/11] SPL: FIT: allow loading multiple images

[Lokesh Vutla]

On Friday 20 January 2017 07:23 AM, Andre Przywara wrote:
> So far we were not using the FIT image format to its full potential:
> The SPL FIT loader was just loading the first image from the /images
> node plus one of the listed DTBs.
> Now with the refactored loader code it's easy to load an arbitrary
> number of images in addition to the two mentioned above.
> As described in the FIT image source file format description, iterate
> over all images listed at the "loadables" property in the configuration
> node and load every image at its desired location.
> This allows to load any kind of images:
> - firmware images to execute before U-Boot proper (for instance
>   ARM Trusted Firmware (ATF))
> - firmware images for management processors (SCP, arisc, ...)
> - firmware images for devices like WiFi controllers
> - bit files for FPGAs
> - additional configuration data
> - kernels and/or ramdisks
> The actual usage of this feature would be platform and/or board specific.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Lokesh Vutla <lokeshvuta@ti.com>

Thanks and regards,
Lokesh

[U-Boot] [RFC PATCH 05/11] tools: mksunxiboot: allow larger SPL binaries

[Andre Przywara]

mksunxiboot limits the size of the resulting SPL binaries to pretty
conservative values to cover all SoCs and all boot media (NAND).
In preparation for supporting modern SoCs without NAND, which may
require a really large SPL, introduce comamnd line parameters to
push the possible SPL size to the limits.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/mksunxiboot.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

diff --git a/tools/mksunxiboot.c b/tools/mksunxiboot.c
index 0f0b003..fa54bce 100644
--- a/tools/mksunxiboot.c
+++ b/tools/mksunxiboot.c
@@ -48,37 +48,58 @@ int gen_check_sum(struct boot_file_head *head_p)
 #define ALIGN(x, a) __ALIGN_MASK((x), (typeof(x))(a)-1)
 #define __ALIGN_MASK(x, mask) (((x)+(mask))&~(mask))
 
-#define SUN4I_SRAM_SIZE 0x7600	/* 0x7748+ is used by BROM */
-#define SRAM_LOAD_MAX_SIZE (SUN4I_SRAM_SIZE - sizeof(struct boot_file_head))
+#define SUN4I_SRAM_SIZE		0x7600	/* 0x7748+ is used by BROM */
+#define SUNXI_MAX_SRAM_SIZE	0x8000
+#define SRAM_LOAD_MAX_SIZE (SUNXI_MAX_SRAM_SIZE - sizeof(struct boot_file_head))
 
 /*
  * BROM (at least on A10 and A20) requires NAND-images to be explicitly aligned
  * to a multiple of 8K, and rejects the image otherwise. MMC-images are fine
- * with 512B blocks. To cater for both, align to the largest of the two.
+ * with 512B blocks.
  */
-#define BLOCK_SIZE 0x2000
+#define BLOCK_SIZE_NAND 0x2000
+#define BLOCK_SIZE_MMC  0x0200
+#define MAX_BLOCK_SIZE	BLOCK_SIZE_NAND
 
 struct boot_img {
 	struct boot_file_head header;
 	char code[SRAM_LOAD_MAX_SIZE];
-	char pad[BLOCK_SIZE];
+	char pad[MAX_BLOCK_SIZE];
 };
 
 int main(int argc, char *argv[])
 {
 	int fd_in, fd_out;
+	int ac = 1;
 	struct boot_img img;
 	unsigned file_size;
+	unsigned size_limit = SUN4I_SRAM_SIZE - sizeof(struct boot_file_head);
+	unsigned block_size = BLOCK_SIZE_NAND;
 	int count;
 
 	if (argc < 2) {
 		printf("\tThis program makes an input bin file to sun4i " \
 		       "bootable image.\n" \
-		       "\tUsage: %s input_file out_putfile\n", argv[0]);
+		       "\tUsage: %s input_file [out_putfile]\n", argv[0]);
 		return EXIT_FAILURE;
 	}
 
-	fd_in = open(argv[1], O_RDONLY);
+	if (!strcmp(argv[ac], "--mmc")) {
+		block_size = BLOCK_SIZE_MMC;
+		ac++;
+	}
+	if (!strcmp(argv[ac], "--nand")) {
+		block_size = BLOCK_SIZE_NAND;
+		ac++;
+	}
+	if (!strcmp(argv[ac], "--max")) {
+		size_limit = SUNXI_MAX_SRAM_SIZE - sizeof(struct boot_file_head);
+		ac++;
+	}
+	if (ac >= argc)
+		return EXIT_FAILURE;
+
+	fd_in = open(argv[ac++], O_RDONLY);
 	if (fd_in < 0) {
 		perror("Open input file");
 		return EXIT_FAILURE;
@@ -89,15 +110,19 @@ int main(int argc, char *argv[])
 	/* get input file size */
 	file_size = lseek(fd_in, 0, SEEK_END);
 
-	if (file_size > SRAM_LOAD_MAX_SIZE) {
+	if (file_size > size_limit) {
 		fprintf(stderr, "ERROR: File too large!\n");
 		return EXIT_FAILURE;
 	}
 
-	fd_out = open(argv[2], O_WRONLY | O_CREAT, 0666);
-	if (fd_out < 0) {
-		perror("Open output file");
-		return EXIT_FAILURE;
+	if (ac >= argc) {
+		fd_out = STDOUT_FILENO;
+	} else {
+		fd_out = open(argv[ac], O_WRONLY | O_CREAT, 0666);
+		if (fd_out < 0) {
+			perror("Open output file");
+			return EXIT_FAILURE;
+		}
 	}
 
 	/* read file to buffer to calculate checksum */
@@ -115,7 +140,7 @@ int main(int argc, char *argv[])
 		 & 0x00FFFFFF);
 	memcpy(img.header.magic, BOOT0_MAGIC, 8);	/* no '0' termination */
 	img.header.length =
-		ALIGN(file_size + sizeof(struct boot_file_head), BLOCK_SIZE);
+		ALIGN(file_size + sizeof(struct boot_file_head), block_size);
 	img.header.b_instruction = cpu_to_le32(img.header.b_instruction);
 	img.header.length = cpu_to_le32(img.header.length);
 
-- 
2.8.2

[U-Boot] [RFC PATCH 05/11] tools: mksunxiboot: allow larger SPL binaries

[Siarhei Siamashka]

On Fri, 20 Jan 2017 01:53:25 +0000
Andre Przywara <andre.przywara@arm.com> wrote:

> mksunxiboot limits the size of the resulting SPL binaries to pretty
> conservative values to cover all SoCs and all boot media (NAND).
> In preparation for supporting modern SoCs without NAND, which may
> require a really large SPL, introduce comamnd line parameters to
> push the possible SPL size to the limits.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Thanks for trying to improve mksunxiboot.

The proposed "--mmc", "--nand" and "--max" options seem to be completely
redundant and serve no real purpose. The 8K alignment is perfectly
fine for any needs and allows to increase the SPL size to 32K (which
is presumably your intention).

The only Allwinner SoC that may potentially benefit from the 512 bytes
alignment is A13:
    https://linux-sunxi.org/BROM#U-Boot_SPL_limitations
But for the sake of simplicity, we can pretend that A13 has the
same 24K SPL size limit as A10 and A20.

That said, I agree that we should fix the mksunxiboot tool and allow
it to create 32K SPL files. Exceeding the SPL size limit is already
checked elsewhere (via the linker script). So mksunxiboot does not
need to enforce the 24K limit even for A10/A20 boards.

> ---
>  tools/mksunxiboot.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/mksunxiboot.c b/tools/mksunxiboot.c
> index 0f0b003..fa54bce 100644
> --- a/tools/mksunxiboot.c
> +++ b/tools/mksunxiboot.c
> @@ -48,37 +48,58 @@ int gen_check_sum(struct boot_file_head *head_p)
>  #define ALIGN(x, a) __ALIGN_MASK((x), (typeof(x))(a)-1)
>  #define __ALIGN_MASK(x, mask) (((x)+(mask))&~(mask))
>  
> -#define SUN4I_SRAM_SIZE 0x7600	/* 0x7748+ is used by BROM */
> -#define SRAM_LOAD_MAX_SIZE (SUN4I_SRAM_SIZE - sizeof(struct boot_file_head))
> +#define SUN4I_SRAM_SIZE		0x7600	/* 0x7748+ is used by BROM */
> +#define SUNXI_MAX_SRAM_SIZE	0x8000
> +#define SRAM_LOAD_MAX_SIZE (SUNXI_MAX_SRAM_SIZE - sizeof(struct boot_file_head))
>  
>  /*
>   * BROM (at least on A10 and A20) requires NAND-images to be explicitly aligned
>   * to a multiple of 8K, and rejects the image otherwise. MMC-images are fine
> - * with 512B blocks. To cater for both, align to the largest of the two.
> + * with 512B blocks.
>   */
> -#define BLOCK_SIZE 0x2000
> +#define BLOCK_SIZE_NAND 0x2000
> +#define BLOCK_SIZE_MMC  0x0200
> +#define MAX_BLOCK_SIZE	BLOCK_SIZE_NAND
>  
>  struct boot_img {
>  	struct boot_file_head header;
>  	char code[SRAM_LOAD_MAX_SIZE];
> -	char pad[BLOCK_SIZE];
> +	char pad[MAX_BLOCK_SIZE];
>  };
>  
>  int main(int argc, char *argv[])
>  {
>  	int fd_in, fd_out;
> +	int ac = 1;
>  	struct boot_img img;
>  	unsigned file_size;
> +	unsigned size_limit = SUN4I_SRAM_SIZE - sizeof(struct boot_file_head);
> +	unsigned block_size = BLOCK_SIZE_NAND;
>  	int count;
>  
>  	if (argc < 2) {
>  		printf("\tThis program makes an input bin file to sun4i " \
>  		       "bootable image.\n" \
> -		       "\tUsage: %s input_file out_putfile\n", argv[0]);
> +		       "\tUsage: %s input_file [out_putfile]\n", argv[0]);
>  		return EXIT_FAILURE;
>  	}
>  
> -	fd_in = open(argv[1], O_RDONLY);
> +	if (!strcmp(argv[ac], "--mmc")) {
> +		block_size = BLOCK_SIZE_MMC;
> +		ac++;
> +	}
> +	if (!strcmp(argv[ac], "--nand")) {
> +		block_size = BLOCK_SIZE_NAND;
> +		ac++;
> +	}
> +	if (!strcmp(argv[ac], "--max")) {
> +		size_limit = SUNXI_MAX_SRAM_SIZE - sizeof(struct boot_file_head);
> +		ac++;
> +	}
> +	if (ac >= argc)
> +		return EXIT_FAILURE;
> +
> +	fd_in = open(argv[ac++], O_RDONLY);
>  	if (fd_in < 0) {
>  		perror("Open input file");
>  		return EXIT_FAILURE;
> @@ -89,15 +110,19 @@ int main(int argc, char *argv[])
>  	/* get input file size */
>  	file_size = lseek(fd_in, 0, SEEK_END);
>  
> -	if (file_size > SRAM_LOAD_MAX_SIZE) {
> +	if (file_size > size_limit) {
>  		fprintf(stderr, "ERROR: File too large!\n");
>  		return EXIT_FAILURE;
>  	}
>  
> -	fd_out = open(argv[2], O_WRONLY | O_CREAT, 0666);
> -	if (fd_out < 0) {
> -		perror("Open output file");
> -		return EXIT_FAILURE;
> +	if (ac >= argc) {
> +		fd_out = STDOUT_FILENO;
> +	} else {
> +		fd_out = open(argv[ac], O_WRONLY | O_CREAT, 0666);
> +		if (fd_out < 0) {
> +			perror("Open output file");
> +			return EXIT_FAILURE;
> +		}
>  	}
>  
>  	/* read file to buffer to calculate checksum */
> @@ -115,7 +140,7 @@ int main(int argc, char *argv[])
>  		 & 0x00FFFFFF);
>  	memcpy(img.header.magic, BOOT0_MAGIC, 8);	/* no '0' termination */
>  	img.header.length =
> -		ALIGN(file_size + sizeof(struct boot_file_head), BLOCK_SIZE);
> +		ALIGN(file_size + sizeof(struct boot_file_head), block_size);
>  	img.header.b_instruction = cpu_to_le32(img.header.b_instruction);
>  	img.header.length = cpu_to_le32(img.header.length);
>  



-- 
Best regards,
Siarhei Siamashka

[U-Boot] [RFC PATCH 05/11] tools: mksunxiboot: allow larger SPL binaries

[André Przywara]

On 21/01/17 04:24, Siarhei Siamashka wrote:

Hi Siarhei,

> On Fri, 20 Jan 2017 01:53:25 +0000
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> mksunxiboot limits the size of the resulting SPL binaries to pretty
>> conservative values to cover all SoCs and all boot media (NAND).
>> In preparation for supporting modern SoCs without NAND, which may
>> require a really large SPL, introduce comamnd line parameters to
>> push the possible SPL size to the limits.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> Thanks for trying to improve mksunxiboot.
> 
> The proposed "--mmc", "--nand" and "--max" options seem to be completely
> redundant and serve no real purpose. The 8K alignment is perfectly
> fine for any needs and allows to increase the SPL size to 32K (which
> is presumably your intention).

Mmmh, interesting, I think my first observation was that adjusting the
limit wasn't good enough, but there was some side effect of the
alignment connected to the limit not being exactly 32K, but slightly
below. So the easiest solution was to just follow the comment, but make
this configurable to not break NAND flash.

But as I can't reproduce this anymore, I am all too happy to drop this part.

> The only Allwinner SoC that may potentially benefit from the 512 bytes
> alignment is A13:
>     https://linux-sunxi.org/BROM#U-Boot_SPL_limitations
> But for the sake of simplicity, we can pretend that A13 has the
> same 24K SPL size limit as A10 and A20.

Fair enough.

> That said, I agree that we should fix the mksunxiboot tool and allow
> it to create 32K SPL files. Exceeding the SPL size limit is already
> checked elsewhere (via the linker script). So mksunxiboot does not
> need to enforce the 24K limit even for A10/A20 boards.

But what about the 0x7600 limit for the older SoCs?:
#define SUN4I_SRAM_SIZE 0x7600  /* 0x7748+ is used by BROM */

Or is this snake oil as we limit earlier to 24K already anyway?

So do you hint that this whole patch can eventually be reduced to:

-#define SUN4I_SRAM_SIZE 0x7600  /* 0x7748+ is used by BROM */
+#define SUN4I_SRAM_SIZE 0x8000

Cheers,
Andre.


>> ---
>>  tools/mksunxiboot.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 38 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/mksunxiboot.c b/tools/mksunxiboot.c
>> index 0f0b003..fa54bce 100644
>> --- a/tools/mksunxiboot.c
>> +++ b/tools/mksunxiboot.c
>> @@ -48,37 +48,58 @@ int gen_check_sum(struct boot_file_head *head_p)
>>  #define ALIGN(x, a) __ALIGN_MASK((x), (typeof(x))(a)-1)
>>  #define __ALIGN_MASK(x, mask) (((x)+(mask))&~(mask))
>>  
>> -#define SUN4I_SRAM_SIZE 0x7600	/* 0x7748+ is used by BROM */
>> -#define SRAM_LOAD_MAX_SIZE (SUN4I_SRAM_SIZE - sizeof(struct boot_file_head))
>> +#define SUN4I_SRAM_SIZE		0x7600	/* 0x7748+ is used by BROM */
>> +#define SUNXI_MAX_SRAM_SIZE	0x8000
>> +#define SRAM_LOAD_MAX_SIZE (SUNXI_MAX_SRAM_SIZE - sizeof(struct boot_file_head))
>>  
>>  /*
>>   * BROM (at least on A10 and A20) requires NAND-images to be explicitly aligned
>>   * to a multiple of 8K, and rejects the image otherwise. MMC-images are fine
>> - * with 512B blocks. To cater for both, align to the largest of the two.
>> + * with 512B blocks.
>>   */
>> -#define BLOCK_SIZE 0x2000
>> +#define BLOCK_SIZE_NAND 0x2000
>> +#define BLOCK_SIZE_MMC  0x0200
>> +#define MAX_BLOCK_SIZE	BLOCK_SIZE_NAND
>>  
>>  struct boot_img {
>>  	struct boot_file_head header;
>>  	char code[SRAM_LOAD_MAX_SIZE];
>> -	char pad[BLOCK_SIZE];
>> +	char pad[MAX_BLOCK_SIZE];
>>  };
>>  
>>  int main(int argc, char *argv[])
>>  {
>>  	int fd_in, fd_out;
>> +	int ac = 1;
>>  	struct boot_img img;
>>  	unsigned file_size;
>> +	unsigned size_limit = SUN4I_SRAM_SIZE - sizeof(struct boot_file_head);
>> +	unsigned block_size = BLOCK_SIZE_NAND;
>>  	int count;
>>  
>>  	if (argc < 2) {
>>  		printf("\tThis program makes an input bin file to sun4i " \
>>  		       "bootable image.\n" \
>> -		       "\tUsage: %s input_file out_putfile\n", argv[0]);
>> +		       "\tUsage: %s input_file [out_putfile]\n", argv[0]);
>>  		return EXIT_FAILURE;
>>  	}
>>  
>> -	fd_in = open(argv[1], O_RDONLY);
>> +	if (!strcmp(argv[ac], "--mmc")) {
>> +		block_size = BLOCK_SIZE_MMC;
>> +		ac++;
>> +	}
>> +	if (!strcmp(argv[ac], "--nand")) {
>> +		block_size = BLOCK_SIZE_NAND;
>> +		ac++;
>> +	}
>> +	if (!strcmp(argv[ac], "--max")) {
>> +		size_limit = SUNXI_MAX_SRAM_SIZE - sizeof(struct boot_file_head);
>> +		ac++;
>> +	}
>> +	if (ac >= argc)
>> +		return EXIT_FAILURE;
>> +
>> +	fd_in = open(argv[ac++], O_RDONLY);
>>  	if (fd_in < 0) {
>>  		perror("Open input file");
>>  		return EXIT_FAILURE;
>> @@ -89,15 +110,19 @@ int main(int argc, char *argv[])
>>  	/* get input file size */
>>  	file_size = lseek(fd_in, 0, SEEK_END);
>>  
>> -	if (file_size > SRAM_LOAD_MAX_SIZE) {
>> +	if (file_size > size_limit) {
>>  		fprintf(stderr, "ERROR: File too large!\n");
>>  		return EXIT_FAILURE;
>>  	}
>>  
>> -	fd_out = open(argv[2], O_WRONLY | O_CREAT, 0666);
>> -	if (fd_out < 0) {
>> -		perror("Open output file");
>> -		return EXIT_FAILURE;
>> +	if (ac >= argc) {
>> +		fd_out = STDOUT_FILENO;
>> +	} else {
>> +		fd_out = open(argv[ac], O_WRONLY | O_CREAT, 0666);
>> +		if (fd_out < 0) {
>> +			perror("Open output file");
>> +			return EXIT_FAILURE;
>> +		}
>>  	}
>>  
>>  	/* read file to buffer to calculate checksum */
>> @@ -115,7 +140,7 @@ int main(int argc, char *argv[])
>>  		 & 0x00FFFFFF);
>>  	memcpy(img.header.magic, BOOT0_MAGIC, 8);	/* no '0' termination */
>>  	img.header.length =
>> -		ALIGN(file_size + sizeof(struct boot_file_head), BLOCK_SIZE);
>> +		ALIGN(file_size + sizeof(struct boot_file_head), block_size);
>>  	img.header.b_instruction = cpu_to_le32(img.header.b_instruction);
>>  	img.header.length = cpu_to_le32(img.header.length);
>>  
> 
> 
> 

[U-Boot] [RFC PATCH 06/11] sunxi: A64: SPL: allow large SPL binary

[Andre Przywara]

Compiling the SPL in AArch64 results in bigger code, which exceeds the
pretty conservative default limits of mksunxiboot.
Use the newly introduced command line parameters to extend the file size
limit to the actual one, which is 32 KB.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 scripts/Makefile.spl | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl
index c962bbc..ee87084 100644
--- a/scripts/Makefile.spl
+++ b/scripts/Makefile.spl
@@ -167,6 +167,11 @@ endif
 
 ifdef CONFIG_ARCH_SUNXI
 ALL-y	+= $(obj)/sunxi-spl.bin
+ifdef CONFIG_MACH_SUN50I
+	MKSUNXIBOOT_PARAMS = --mmc --max
+else
+	MKSUNXIBOOT_PARAMS =
+endif
 endif
 
 ifeq ($(CONFIG_SYS_SOC),"at91")
@@ -271,7 +276,7 @@ $(obj)/$(SPL_BIN).sfp: $(obj)/$(SPL_BIN).bin FORCE
 	$(call if_changed,mkimage)
 
 quiet_cmd_mksunxiboot = MKSUNXI $@
-cmd_mksunxiboot = $(objtree)/tools/mksunxiboot $< $@
+cmd_mksunxiboot = $(objtree)/tools/mksunxiboot $(MKSUNXIBOOT_PARAMS) $< $@
 $(obj)/sunxi-spl.bin: $(obj)/$(SPL_BIN).bin FORCE
 	$(call if_changed,mksunxiboot)
 
-- 
2.8.2

[U-Boot] [RFC PATCH 07/11] sunxi: A64: move SPL stack to end of SRAM A2

[Andre Przywara]

The SPL stack is usually located at the end of SRAM A1, where it grows
towards the end of the SPL.
For the really big AArch64 binaries the stack overwrites code pretty
soon, so move the SPL stack to the end of SRAM A2, which is unused at this
time.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/configs/sunxi-common.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index d58e5ba..28878a5 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -203,10 +203,14 @@
 
 #define CONFIG_SPL_PAD_TO		32768		/* decimal for 'dd' */
 
-#if defined(CONFIG_MACH_SUN9I) || defined(CONFIG_MACH_SUN50I)
+#if defined(CONFIG_MACH_SUN9I)
 /* FIXME: 40 KiB instead of 32 KiB ? */
 #define LOW_LEVEL_SRAM_STACK		0x00018000
 #define CONFIG_SPL_STACK		LOW_LEVEL_SRAM_STACK
+#elif defined(CONFIG_MACH_SUN50I)
+/* end of SRAM A2 for now, as SRAM A1 is pretty tight */
+#define LOW_LEVEL_SRAM_STACK		0x00054000
+#define CONFIG_SPL_STACK		LOW_LEVEL_SRAM_STACK
 #else
 /* end of 32 KiB in sram */
 #define LOW_LEVEL_SRAM_STACK		0x00008000 /* End of sram */
-- 
2.8.2

[U-Boot] [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[Andre Przywara]

For a board or platform to support FIT loading in the SPL, it has to
provide a board_fit_config_name_match() routine, which helps to select
one of possibly multiple DTBs contained in a FIT image.
Provide a simple function to cover the two different Pine64 models,
which can be easily told apart by looking at the amount of installed
RAM.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 board/sunxi/board.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 5365638..bbbb826 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)
 #endif
 	return 0;
 }
+
+#ifdef CONFIG_SPL_LOAD_FIT
+int board_fit_config_name_match(const char *name)
+{
+#ifdef CONFIG_MACH_SUN50I
+	if ((gd->ram_size > 512 * 1024 * 1024))
+		return !strcmp(name, "sun50i-a64-pine64-plus");
+	else
+		return !strcmp(name, "sun50i-a64-pine64");
+#endif
+	return -1;
+}
+#endif
-- 
2.8.2

[U-Boot] [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[Maxime Ripard]

Hi,

On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:
> For a board or platform to support FIT loading in the SPL, it has to
> provide a board_fit_config_name_match() routine, which helps to select
> one of possibly multiple DTBs contained in a FIT image.
> Provide a simple function to cover the two different Pine64 models,
> which can be easily told apart by looking at the amount of installed
> RAM.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  board/sunxi/board.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 5365638..bbbb826 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)
>  #endif
>  	return 0;
>  }
> +
> +#ifdef CONFIG_SPL_LOAD_FIT
> +int board_fit_config_name_match(const char *name)
> +{
> +#ifdef CONFIG_MACH_SUN50I
> +	if ((gd->ram_size > 512 * 1024 * 1024))
> +		return !strcmp(name, "sun50i-a64-pine64-plus");
> +	else
> +		return !strcmp(name, "sun50i-a64-pine64");
> +#endif
> +	return -1;
> +}

That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT
enabled will be considered a pine64 board?

Maxime


-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170120/d1e235d5/attachment.sig>

[U-Boot] [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[André Przywara]

On 20/01/17 21:35, Maxime Ripard wrote:

Hi Maxime,

thanks for having a look!

> On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:
>> For a board or platform to support FIT loading in the SPL, it has to
>> provide a board_fit_config_name_match() routine, which helps to select
>> one of possibly multiple DTBs contained in a FIT image.
>> Provide a simple function to cover the two different Pine64 models,
>> which can be easily told apart by looking at the amount of installed
>> RAM.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  board/sunxi/board.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index 5365638..bbbb826 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)
>>  #endif
>>  	return 0;
>>  }
>> +
>> +#ifdef CONFIG_SPL_LOAD_FIT
>> +int board_fit_config_name_match(const char *name)
>> +{
>> +#ifdef CONFIG_MACH_SUN50I
>> +	if ((gd->ram_size > 512 * 1024 * 1024))
>> +		return !strcmp(name, "sun50i-a64-pine64-plus");
>> +	else
>> +		return !strcmp(name, "sun50i-a64-pine64");
>> +#endif
>> +	return -1;
>> +}
> 
> That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT
> enabled will be considered a pine64 board?

Yes, at least for now, because that's the only A64 board we officially
support so far.
And yes again, it is fishy. It is more a demo or a placeholder for now,
because you _need_ an implementation of board_fit_config_name_match()
once you enable CONFIG_SPL_LOAD_FIT.

One solution would be to have only one DTB supported per build, so
board_fit_config_name_match() always returns 0.

Another (better) solution would be to store the board name in the SPL
header, which could be done later when flashing the image. Then this
function would just return strcmp(name, spl_header_name) or 0 if no name
is found for whatever reason.

Ideas welcome. To be honest, this .dtb selection is nice, but I am not
married to it. It usefulness maybe limited at the moment.

Cheers,
Andre.

[U-Boot] [linux-sunxi] Re: [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[Icenowy Zheng]

21.01.2017, 05:56, "Andr? Przywara" <andre.przywara@arm.com>:
> On 20/01/17 21:35, Maxime Ripard wrote:
>
> Hi Maxime,
>
> thanks for having a look!
>
>> ?On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:
>>> ?For a board or platform to support FIT loading in the SPL, it has to
>>> ?provide a board_fit_config_name_match() routine, which helps to select
>>> ?one of possibly multiple DTBs contained in a FIT image.
>>> ?Provide a simple function to cover the two different Pine64 models,
>>> ?which can be easily told apart by looking at the amount of installed
>>> ?RAM.
>>>
>>> ?Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ?---
>>> ??board/sunxi/board.c | 13 +++++++++++++
>>> ??1 file changed, 13 insertions(+)
>>>
>>> ?diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>>> ?index 5365638..bbbb826 100644
>>> ?--- a/board/sunxi/board.c
>>> ?+++ b/board/sunxi/board.c
>>> ?@@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)
>>> ??#endif
>>> ??????????return 0;
>>> ??}
>>> ?+
>>> ?+#ifdef CONFIG_SPL_LOAD_FIT
>>> ?+int board_fit_config_name_match(const char *name)
>>> ?+{
>>> ?+#ifdef CONFIG_MACH_SUN50I
>>> ?+ if ((gd->ram_size > 512 * 1024 * 1024))
>>> ?+ return !strcmp(name, "sun50i-a64-pine64-plus");
>>> ?+ else
>>> ?+ return !strcmp(name, "sun50i-a64-pine64");
>>> ?+#endif
>>> ?+ return -1;
>>> ?+}
>>
>> ?That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT
>> ?enabled will be considered a pine64 board?
>
> Yes, at least for now, because that's the only A64 board we officially
> support so far.
> And yes again, it is fishy. It is more a demo or a placeholder for now,
> because you _need_ an implementation of board_fit_config_name_match()
> once you enable CONFIG_SPL_LOAD_FIT.

You have a Banana Pi M64, right?

And I do not want to change code every time we have a new A64 board...

> One solution would be to have only one DTB supported per build, so
> board_fit_config_name_match() always returns 0.
>

I think that's what we're did in previous u-boot-sunxi-with-spl.bin .

> Another (better) solution would be to store the board name in the SPL
> header, which could be done later when flashing the image. Then this
> function would just return strcmp(name, spl_header_name) or 0 if no name
> is found for whatever reason.
>
> Ideas welcome. To be honest, this .dtb selection is nice, but I am not
> married to it. It usefulness maybe limited at the moment.
>
> Cheers,
> Andre.
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[U-Boot] [linux-sunxi] Re: [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[Icenowy Zheng]

21.01.2017, 05:56, "Andr? Przywara" <andre.przywara@arm.com>:
> On 20/01/17 21:35, Maxime Ripard wrote:
>
> Hi Maxime,
>
> thanks for having a look!
>
>> ?On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:
>>> ?For a board or platform to support FIT loading in the SPL, it has to
>>> ?provide a board_fit_config_name_match() routine, which helps to select
>>> ?one of possibly multiple DTBs contained in a FIT image.
>>> ?Provide a simple function to cover the two different Pine64 models,
>>> ?which can be easily told apart by looking at the amount of installed
>>> ?RAM.
>>>
>>> ?Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ?---
>>> ??board/sunxi/board.c | 13 +++++++++++++
>>> ??1 file changed, 13 insertions(+)
>>>
>>> ?diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>>> ?index 5365638..bbbb826 100644
>>> ?--- a/board/sunxi/board.c
>>> ?+++ b/board/sunxi/board.c
>>> ?@@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)
>>> ??#endif
>>> ??????????return 0;
>>> ??}
>>> ?+
>>> ?+#ifdef CONFIG_SPL_LOAD_FIT
>>> ?+int board_fit_config_name_match(const char *name)
>>> ?+{
>>> ?+#ifdef CONFIG_MACH_SUN50I
>>> ?+ if ((gd->ram_size > 512 * 1024 * 1024))
>>> ?+ return !strcmp(name, "sun50i-a64-pine64-plus");
>>> ?+ else
>>> ?+ return !strcmp(name, "sun50i-a64-pine64");
>>> ?+#endif
>>> ?+ return -1;
>>> ?+}
>>
>> ?That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT
>> ?enabled will be considered a pine64 board?
>
> Yes, at least for now, because that's the only A64 board we officially
> support so far.
> And yes again, it is fishy. It is more a demo or a placeholder for now,
> because you _need_ an implementation of board_fit_config_name_match()
> once you enable CONFIG_SPL_LOAD_FIT.

You have a Banana Pi M64, right?

And I do not want to change code every time we have a new A64 board...

> One solution would be to have only one DTB supported per build, so
> board_fit_config_name_match() always returns 0.
>

I think that's what we're did in previous u-boot-sunxi-with-spl.bin .

> Another (better) solution would be to store the board name in the SPL
> header, which could be done later when flashing the image. Then this
> function would just return strcmp(name, spl_header_name) or 0 if no name
> is found for whatever reason.
>
> Ideas welcome. To be honest, this .dtb selection is nice, but I am not
> married to it. It usefulness maybe limited at the moment.
>
> Cheers,
> Andre.
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[U-Boot] [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[Siarhei Siamashka]

On Fri, 20 Jan 2017 21:55:53 +0000
Andr? Przywara <andre.przywara@arm.com> wrote:

> On 20/01/17 21:35, Maxime Ripard wrote:
> 
> Hi Maxime,
> 
> thanks for having a look!
> 
> > On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:  
> >> For a board or platform to support FIT loading in the SPL, it has to
> >> provide a board_fit_config_name_match() routine, which helps to select
> >> one of possibly multiple DTBs contained in a FIT image.
> >> Provide a simple function to cover the two different Pine64 models,
> >> which can be easily told apart by looking at the amount of installed
> >> RAM.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >>  board/sunxi/board.c | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>
> >> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> >> index 5365638..bbbb826 100644
> >> --- a/board/sunxi/board.c
> >> +++ b/board/sunxi/board.c
> >> @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)
> >>  #endif
> >>  	return 0;
> >>  }
> >> +
> >> +#ifdef CONFIG_SPL_LOAD_FIT
> >> +int board_fit_config_name_match(const char *name)
> >> +{
> >> +#ifdef CONFIG_MACH_SUN50I
> >> +	if ((gd->ram_size > 512 * 1024 * 1024))
> >> +		return !strcmp(name, "sun50i-a64-pine64-plus");
> >> +	else
> >> +		return !strcmp(name, "sun50i-a64-pine64");
> >> +#endif
> >> +	return -1;
> >> +}  
> > 
> > That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT
> > enabled will be considered a pine64 board?  
> 
> Yes, at least for now, because that's the only A64 board we officially
> support so far.
> And yes again, it is fishy. It is more a demo or a placeholder for now,
> because you _need_ an implementation of board_fit_config_name_match()
> once you enable CONFIG_SPL_LOAD_FIT.
> 
> One solution would be to have only one DTB supported per build, so
> board_fit_config_name_match() always returns 0.
> 
> Another (better) solution would be to store the board name in the SPL
> header, which could be done later when flashing the image. Then this
> function would just return strcmp(name, spl_header_name) or 0 if no name
> is found for whatever reason.

Yes, this is a good solution in the case if we have some non-removable
storage on the board (SPI NOR flash, NAND, EEPROM or something else).
We can discuss it separately.

But the current generation of Pine64 boards don't have any
non-removable storage yet. My understanding is that you tried to
provide the DTB selection, which is based on circumstantial evidences,
such as the RAM size. IMHO this is also a valid selection method,
because it can reduce the number of required OS image variants.

Yes, this is not urgent and we can indeed temporarily use separate
defconfigs for different Pine64 board variants.

> Ideas welcome. To be honest, this .dtb selection is nice, but I am not
> married to it. It usefulness maybe limited at the moment.

The board specific information is normally stored either in the
defconfig file or in the device tree, rather than gets hardcoded in
the sources. We can probably add some extra constraints information
to the device tree. And these extra constraints can be passed to the
board_fit_config_name_match() function in some way.

Then the sun50i-a64-pine64 device tree file can specify that this board
is expected to have exactly 512 MiB of RAM. Having this information,
the board_fit_config_name_match() function will fail to match it if
the actual RAM size is wrong.

This approach is similar to what I used in
    https://github.com/ssvb/sunxi-bootsetup/releases/tag/20141215-sunxi-bootsetup-prototype
    http://lists.denx.de/pipermail/u-boot/2015-January/202306.html
    
The sunxi-bootsetup stub used the SoC type id and RAM size to filter
the list of potentially matching boards. It worked very nicely in
practice.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[André Przywara]

On 21/01/17 04:05, Siarhei Siamashka wrote:

Hi Siarhei,

thanks for your comments!

> On Fri, 20 Jan 2017 21:55:53 +0000
> Andr? Przywara <andre.przywara@arm.com> wrote:
> 
>> On 20/01/17 21:35, Maxime Ripard wrote:
>>
>> Hi Maxime,
>>
>> thanks for having a look!
>>
>>> On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:  
>>>> For a board or platform to support FIT loading in the SPL, it has to
>>>> provide a board_fit_config_name_match() routine, which helps to select
>>>> one of possibly multiple DTBs contained in a FIT image.
>>>> Provide a simple function to cover the two different Pine64 models,
>>>> which can be easily told apart by looking at the amount of installed
>>>> RAM.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>>  board/sunxi/board.c | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>>>> index 5365638..bbbb826 100644
>>>> --- a/board/sunxi/board.c
>>>> +++ b/board/sunxi/board.c
>>>> @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)
>>>>  #endif
>>>>  	return 0;
>>>>  }
>>>> +
>>>> +#ifdef CONFIG_SPL_LOAD_FIT
>>>> +int board_fit_config_name_match(const char *name)
>>>> +{
>>>> +#ifdef CONFIG_MACH_SUN50I
>>>> +	if ((gd->ram_size > 512 * 1024 * 1024))
>>>> +		return !strcmp(name, "sun50i-a64-pine64-plus");
>>>> +	else
>>>> +		return !strcmp(name, "sun50i-a64-pine64");
>>>> +#endif
>>>> +	return -1;
>>>> +}  
>>>
>>> That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT
>>> enabled will be considered a pine64 board?  
>>
>> Yes, at least for now, because that's the only A64 board we officially
>> support so far.
>> And yes again, it is fishy. It is more a demo or a placeholder for now,
>> because you _need_ an implementation of board_fit_config_name_match()
>> once you enable CONFIG_SPL_LOAD_FIT.
>>
>> One solution would be to have only one DTB supported per build, so
>> board_fit_config_name_match() always returns 0.
>>
>> Another (better) solution would be to store the board name in the SPL
>> header, which could be done later when flashing the image. Then this
>> function would just return strcmp(name, spl_header_name) or 0 if no name
>> is found for whatever reason.
> 
> Yes, this is a good solution in the case if we have some non-removable
> storage on the board (SPI NOR flash, NAND, EEPROM or something else).
> We can discuss it separately.

I totally agree. I rebased your patch to latest mainline already and
will send out something later.

> But the current generation of Pine64 boards don't have any
> non-removable storage yet. My understanding is that you tried to
> provide the DTB selection, which is based on circumstantial evidences,
> such as the RAM size. IMHO this is also a valid selection method,
> because it can reduce the number of required OS image variants.

Yes, the point is that for U-Boot's purposes the only difference between
the Pine64 and Pine64+ (apart from DRAM size, which is autodetected) is
using MII vs. RGMII for the Ethernet PHY. The sun8i_emac driver gets
this information from the DT only, so there is no point at all for
different defconfigs. And the DRAM size is a safe indicator for that
difference, at least if we confine our view to Pine64 boards.

Now how other boards fit in here is a separate discussion IMO, so let's
solve one problem after the other.
I just thought that we could use:

#ifdef CONFIG_SPL_LOAD_FIT
int board_fit_config_name_match(const char *name)
{
	return strcmp(name, CONFIG_DEFAULT_DEVICE_TREE);
}
#endif

for any general case.

> Yes, this is not urgent and we can indeed temporarily use separate
> defconfigs for different Pine64 board variants.

I agree it's not a showstopper and we can work out a solution later.
But in the long run I'd really like to have one firmware build suitable
for the two Pine64 models, so we just need to figure out how to fit the
DRAM size comparison code in here, maybe via some board specific #define
to use the routine from this patch vs. the generic routine above?

Btw: I think I tested a non-plus Pine64 some time ago and (Fast)
Ethernet didn't work out of the box back then, so I guess at the moment
we support the Pine64+ only anyway.

>> Ideas welcome. To be honest, this .dtb selection is nice, but I am not
>> married to it. It usefulness maybe limited at the moment.
> 
> The board specific information is normally stored either in the
> defconfig file or in the device tree, rather than gets hardcoded in
> the sources. We can probably add some extra constraints information
> to the device tree. And these extra constraints can be passed to the
> board_fit_config_name_match() function in some way.
> 
> Then the sun50i-a64-pine64 device tree file can specify that this board
> is expected to have exactly 512 MiB of RAM. Having this information,
> the board_fit_config_name_match() function will fail to match it if
> the actual RAM size is wrong.

The problem is that at the moment this function gets only passed the
configuration _name_ from the FIT property, which happens to be the DT
name. At this point the DT is not loaded yet, so the function cannot
take the actual DT into account (if this what you are hinting at).

And I am not sure if it's worth to actually load any DTs you want to
check and do the actual comparison with the DT and not just the name.
Sounds like a bit of a stretch to me.

> This approach is similar to what I used in
>     https://github.com/ssvb/sunxi-bootsetup/releases/tag/20141215-sunxi-bootsetup-prototype
>     http://lists.denx.de/pipermail/u-boot/2015-January/202306.html

Personally I am a big fan of using one image to rule them all (or at
least many of them), but I don't think we are there yet. Also it dos not
seems to be very popular with others.
So let's get the basics upstream first and then take a look at how we
can achieve this.

Are you around at FOSDEM? Then we could discuss this there more efficiently.

> The sunxi-bootsetup stub used the SoC type id and RAM size to filter
> the list of potentially matching boards. It worked very nicely in
> practice.

I need to check your code and the email threads from back then, but in
general I agree that this is the ultimate goal.

Cheers,
Andre.

[U-Boot] [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[Maxime Ripard]

On Sat, Jan 21, 2017 at 03:15:27PM +0000, Andr? Przywara wrote:
> >>> On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:  
> >>>> For a board or platform to support FIT loading in the SPL, it has to
> >>>> provide a board_fit_config_name_match() routine, which helps to select
> >>>> one of possibly multiple DTBs contained in a FIT image.
> >>>> Provide a simple function to cover the two different Pine64 models,
> >>>> which can be easily told apart by looking at the amount of installed
> >>>> RAM.
> >>>>
> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >>>> ---
> >>>>  board/sunxi/board.c | 13 +++++++++++++
> >>>>  1 file changed, 13 insertions(+)
> >>>>
> >>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> >>>> index 5365638..bbbb826 100644
> >>>> --- a/board/sunxi/board.c
> >>>> +++ b/board/sunxi/board.c
> >>>> @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)
> >>>>  #endif
> >>>>  	return 0;
> >>>>  }
> >>>> +
> >>>> +#ifdef CONFIG_SPL_LOAD_FIT
> >>>> +int board_fit_config_name_match(const char *name)
> >>>> +{
> >>>> +#ifdef CONFIG_MACH_SUN50I
> >>>> +	if ((gd->ram_size > 512 * 1024 * 1024))
> >>>> +		return !strcmp(name, "sun50i-a64-pine64-plus");
> >>>> +	else
> >>>> +		return !strcmp(name, "sun50i-a64-pine64");
> >>>> +#endif
> >>>> +	return -1;
> >>>> +}  
> >>>
> >>> That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT
> >>> enabled will be considered a pine64 board?  
> >>
> >> Yes, at least for now, because that's the only A64 board we officially
> >> support so far.
> >> And yes again, it is fishy. It is more a demo or a placeholder for now,
> >> because you _need_ an implementation of board_fit_config_name_match()
> >> once you enable CONFIG_SPL_LOAD_FIT.
> >>
> >> One solution would be to have only one DTB supported per build, so
> >> board_fit_config_name_match() always returns 0.
> >>
> >> Another (better) solution would be to store the board name in the SPL
> >> header, which could be done later when flashing the image. Then this
> >> function would just return strcmp(name, spl_header_name) or 0 if no name
> >> is found for whatever reason.
> > 
> > Yes, this is a good solution in the case if we have some non-removable
> > storage on the board (SPI NOR flash, NAND, EEPROM or something else).
> > We can discuss it separately.
> 
> I totally agree. I rebased your patch to latest mainline already and
> will send out something later.
> 
> > But the current generation of Pine64 boards don't have any
> > non-removable storage yet. My understanding is that you tried to
> > provide the DTB selection, which is based on circumstantial evidences,
> > such as the RAM size. IMHO this is also a valid selection method,
> > because it can reduce the number of required OS image variants.
> 
> Yes, the point is that for U-Boot's purposes the only difference between
> the Pine64 and Pine64+ (apart from DRAM size, which is autodetected) is
> using MII vs. RGMII for the Ethernet PHY. The sun8i_emac driver gets
> this information from the DT only, so there is no point at all for
> different defconfigs. And the DRAM size is a safe indicator for that
> difference, at least if we confine our view to Pine64 boards.
> 
> Now how other boards fit in here is a separate discussion IMO, so let's
> solve one problem after the other.
> I just thought that we could use:
> 
> #ifdef CONFIG_SPL_LOAD_FIT
> int board_fit_config_name_match(const char *name)
> {
> 	return strcmp(name, CONFIG_DEFAULT_DEVICE_TREE);
> }
> #endif

I guess an easy way around this would be to add a Kconfig option for
the pine64, like I tried to do for the CHIP (but never ended up
merging). That way, at least we won't impact the other board, and we
can have that default.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170123/173df7a4/attachment.sig>

[U-Boot] [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[André Przywara]

On 23/01/17 17:29, Maxime Ripard wrote:
> On Sat, Jan 21, 2017 at 03:15:27PM +0000, Andr? Przywara wrote:
>>>>> On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:  
>>>>>> For a board or platform to support FIT loading in the SPL, it has to
>>>>>> provide a board_fit_config_name_match() routine, which helps to select
>>>>>> one of possibly multiple DTBs contained in a FIT image.
>>>>>> Provide a simple function to cover the two different Pine64 models,
>>>>>> which can be easily told apart by looking at the amount of installed
>>>>>> RAM.
>>>>>>
>>>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>>>> ---
>>>>>>  board/sunxi/board.c | 13 +++++++++++++
>>>>>>  1 file changed, 13 insertions(+)
>>>>>>
>>>>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>>>>>> index 5365638..bbbb826 100644
>>>>>> --- a/board/sunxi/board.c
>>>>>> +++ b/board/sunxi/board.c
>>>>>> @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)
>>>>>>  #endif
>>>>>>  	return 0;
>>>>>>  }
>>>>>> +
>>>>>> +#ifdef CONFIG_SPL_LOAD_FIT
>>>>>> +int board_fit_config_name_match(const char *name)
>>>>>> +{
>>>>>> +#ifdef CONFIG_MACH_SUN50I
>>>>>> +	if ((gd->ram_size > 512 * 1024 * 1024))
>>>>>> +		return !strcmp(name, "sun50i-a64-pine64-plus");
>>>>>> +	else
>>>>>> +		return !strcmp(name, "sun50i-a64-pine64");
>>>>>> +#endif
>>>>>> +	return -1;
>>>>>> +}  
>>>>>
>>>>> That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT
>>>>> enabled will be considered a pine64 board?  
>>>>
>>>> Yes, at least for now, because that's the only A64 board we officially
>>>> support so far.
>>>> And yes again, it is fishy. It is more a demo or a placeholder for now,
>>>> because you _need_ an implementation of board_fit_config_name_match()
>>>> once you enable CONFIG_SPL_LOAD_FIT.
>>>>
>>>> One solution would be to have only one DTB supported per build, so
>>>> board_fit_config_name_match() always returns 0.
>>>>
>>>> Another (better) solution would be to store the board name in the SPL
>>>> header, which could be done later when flashing the image. Then this
>>>> function would just return strcmp(name, spl_header_name) or 0 if no name
>>>> is found for whatever reason.
>>>
>>> Yes, this is a good solution in the case if we have some non-removable
>>> storage on the board (SPI NOR flash, NAND, EEPROM or something else).
>>> We can discuss it separately.
>>
>> I totally agree. I rebased your patch to latest mainline already and
>> will send out something later.
>>
>>> But the current generation of Pine64 boards don't have any
>>> non-removable storage yet. My understanding is that you tried to
>>> provide the DTB selection, which is based on circumstantial evidences,
>>> such as the RAM size. IMHO this is also a valid selection method,
>>> because it can reduce the number of required OS image variants.
>>
>> Yes, the point is that for U-Boot's purposes the only difference between
>> the Pine64 and Pine64+ (apart from DRAM size, which is autodetected) is
>> using MII vs. RGMII for the Ethernet PHY. The sun8i_emac driver gets
>> this information from the DT only, so there is no point at all for
>> different defconfigs. And the DRAM size is a safe indicator for that
>> difference, at least if we confine our view to Pine64 boards.
>>
>> Now how other boards fit in here is a separate discussion IMO, so let's
>> solve one problem after the other.
>> I just thought that we could use:
>>
>> #ifdef CONFIG_SPL_LOAD_FIT
>> int board_fit_config_name_match(const char *name)
>> {
>> 	return strcmp(name, CONFIG_DEFAULT_DEVICE_TREE);
>> }
>> #endif
> 
> I guess an easy way around this would be to add a Kconfig option for
> the pine64, like I tried to do for the CHIP (but never ended up
> merging). That way, at least we won't impact the other board, and we
> can have that default.

Yes, that sounds indeed like a possibility.

I came up with this snippet yesterday (rough copy):

/* Check if there is a DT name stored in the SPL header and use that. */
        if (spl->offset_default_dt_name) {
                cmp_str = SPL_ADDR + spl->offset_default_dt_name;
        } else {
#ifdef CONFIG_DEFAULT_DEVICE_TREE
                cmp_str = CONFIG_DEFAULT_DEVICE_TREE;
#else
                return 0;
#endif
        };

        /* Differentiate the two Pine64 board DTs by their DRAM size. */
        if (strstr(name, "-pine64") && strstr(cmp_str, "-pine64")) {
                if ((gd->ram_size > 512 * 1024 * 1024))
                        return !strstr(name, "plus");
                else
                        return !!strstr(name, "plus");
        } else {
                return strcmp(name, cmp_str);
        }

That admittedly doesn't look very pretty, but should cover all the
cases: DT name from SPL header, default compile time DT, Pine64 check.
We could also guard the last part with a CONFIG_SUNXI_PINE64_DETECT
symbol to save some space in case this is not needed.

Does that make sense?

Cheers,
Andre.

[U-Boot] [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[Maxime Ripard]

On Mon, Jan 23, 2017 at 10:55:55PM +0000, Andr? Przywara wrote:
> On 23/01/17 17:29, Maxime Ripard wrote:
> > On Sat, Jan 21, 2017 at 03:15:27PM +0000, Andr? Przywara wrote:
> >>>>> On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:  
> >>>>>> For a board or platform to support FIT loading in the SPL, it has to
> >>>>>> provide a board_fit_config_name_match() routine, which helps to select
> >>>>>> one of possibly multiple DTBs contained in a FIT image.
> >>>>>> Provide a simple function to cover the two different Pine64 models,
> >>>>>> which can be easily told apart by looking at the amount of installed
> >>>>>> RAM.
> >>>>>>
> >>>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >>>>>> ---
> >>>>>>  board/sunxi/board.c | 13 +++++++++++++
> >>>>>>  1 file changed, 13 insertions(+)
> >>>>>>
> >>>>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> >>>>>> index 5365638..bbbb826 100644
> >>>>>> --- a/board/sunxi/board.c
> >>>>>> +++ b/board/sunxi/board.c
> >>>>>> @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)
> >>>>>>  #endif
> >>>>>>  	return 0;
> >>>>>>  }
> >>>>>> +
> >>>>>> +#ifdef CONFIG_SPL_LOAD_FIT
> >>>>>> +int board_fit_config_name_match(const char *name)
> >>>>>> +{
> >>>>>> +#ifdef CONFIG_MACH_SUN50I
> >>>>>> +	if ((gd->ram_size > 512 * 1024 * 1024))
> >>>>>> +		return !strcmp(name, "sun50i-a64-pine64-plus");
> >>>>>> +	else
> >>>>>> +		return !strcmp(name, "sun50i-a64-pine64");
> >>>>>> +#endif
> >>>>>> +	return -1;
> >>>>>> +}  
> >>>>>
> >>>>> That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT
> >>>>> enabled will be considered a pine64 board?  
> >>>>
> >>>> Yes, at least for now, because that's the only A64 board we officially
> >>>> support so far.
> >>>> And yes again, it is fishy. It is more a demo or a placeholder for now,
> >>>> because you _need_ an implementation of board_fit_config_name_match()
> >>>> once you enable CONFIG_SPL_LOAD_FIT.
> >>>>
> >>>> One solution would be to have only one DTB supported per build, so
> >>>> board_fit_config_name_match() always returns 0.
> >>>>
> >>>> Another (better) solution would be to store the board name in the SPL
> >>>> header, which could be done later when flashing the image. Then this
> >>>> function would just return strcmp(name, spl_header_name) or 0 if no name
> >>>> is found for whatever reason.
> >>>
> >>> Yes, this is a good solution in the case if we have some non-removable
> >>> storage on the board (SPI NOR flash, NAND, EEPROM or something else).
> >>> We can discuss it separately.
> >>
> >> I totally agree. I rebased your patch to latest mainline already and
> >> will send out something later.
> >>
> >>> But the current generation of Pine64 boards don't have any
> >>> non-removable storage yet. My understanding is that you tried to
> >>> provide the DTB selection, which is based on circumstantial evidences,
> >>> such as the RAM size. IMHO this is also a valid selection method,
> >>> because it can reduce the number of required OS image variants.
> >>
> >> Yes, the point is that for U-Boot's purposes the only difference between
> >> the Pine64 and Pine64+ (apart from DRAM size, which is autodetected) is
> >> using MII vs. RGMII for the Ethernet PHY. The sun8i_emac driver gets
> >> this information from the DT only, so there is no point at all for
> >> different defconfigs. And the DRAM size is a safe indicator for that
> >> difference, at least if we confine our view to Pine64 boards.
> >>
> >> Now how other boards fit in here is a separate discussion IMO, so let's
> >> solve one problem after the other.
> >> I just thought that we could use:
> >>
> >> #ifdef CONFIG_SPL_LOAD_FIT
> >> int board_fit_config_name_match(const char *name)
> >> {
> >> 	return strcmp(name, CONFIG_DEFAULT_DEVICE_TREE);
> >> }
> >> #endif
> > 
> > I guess an easy way around this would be to add a Kconfig option for
> > the pine64, like I tried to do for the CHIP (but never ended up
> > merging). That way, at least we won't impact the other board, and we
> > can have that default.
> 
> Yes, that sounds indeed like a possibility.
> 
> I came up with this snippet yesterday (rough copy):
> 
> /* Check if there is a DT name stored in the SPL header and use that. */
>         if (spl->offset_default_dt_name) {
>                 cmp_str = SPL_ADDR + spl->offset_default_dt_name;
>         } else {
> #ifdef CONFIG_DEFAULT_DEVICE_TREE
>                 cmp_str = CONFIG_DEFAULT_DEVICE_TREE;
> #else
>                 return 0;
> #endif
>         };
> 
>         /* Differentiate the two Pine64 board DTs by their DRAM size. */
>         if (strstr(name, "-pine64") && strstr(cmp_str, "-pine64")) {
>                 if ((gd->ram_size > 512 * 1024 * 1024))
>                         return !strstr(name, "plus");
>                 else
>                         return !!strstr(name, "plus");
>         } else {
>                 return strcmp(name, cmp_str);
>         }
> 
> That admittedly doesn't look very pretty, but should cover all the
> cases: DT name from SPL header, default compile time DT, Pine64 check.
> We could also guard the last part with a CONFIG_SUNXI_PINE64_DETECT
> symbol to save some space in case this is not needed.
> 
> Does that make sense?

Yep, that works for me (with the config option)

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170126/493de45e/attachment.sig>

[U-Boot] [RFC PATCH 09/11] sunxi: Pine64: defconfig: enable SPL FIT support

[Andre Przywara]

The Pine64 (as all 64-bit Allwinner boards so far) need to load an
ARM Trusted Firmware image beside the actual U-Boot proper.
This can now be easily achieved by using the just extended SPL FIT
loading support, so enable it in the Pine64 defconfig.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 configs/pine64_plus_defconfig | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/configs/pine64_plus_defconfig b/configs/pine64_plus_defconfig
index 2374170..e050800 100644
--- a/configs/pine64_plus_defconfig
+++ b/configs/pine64_plus_defconfig
@@ -3,9 +3,14 @@ CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER=y
 CONFIG_ARCH_SUNXI=y
 CONFIG_MACH_SUN50I=y
 CONFIG_DEFAULT_DEVICE_TREE="sun50i-a64-pine64-plus"
+CONFIG_OF_LIST="sun50i-a64-pine64 sun50i-a64-pine64-plus"
 # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
 CONFIG_CONSOLE_MUX=y
 CONFIG_SPL=y
+CONFIG_FIT=y
+CONFIG_SPL_FIT=y
+CONFIG_SPL_LOAD_FIT=y
+CONFIG_SPL_OF_LIBFDT=y
 # CONFIG_CMD_IMLS is not set
 # CONFIG_CMD_FLASH is not set
 # CONFIG_CMD_FPGA is not set
-- 
2.8.2

[U-Boot] [RFC PATCH 09/11] sunxi: Pine64: defconfig: enable SPL FIT support

[Maxime Ripard]

On Fri, Jan 20, 2017 at 01:53:29AM +0000, Andre Przywara wrote:
> The Pine64 (as all 64-bit Allwinner boards so far) need to load an
> ARM Trusted Firmware image beside the actual U-Boot proper.
> This can now be easily achieved by using the just extended SPL FIT
> loading support, so enable it in the Pine64 defconfig.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  configs/pine64_plus_defconfig | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/configs/pine64_plus_defconfig b/configs/pine64_plus_defconfig
> index 2374170..e050800 100644
> --- a/configs/pine64_plus_defconfig
> +++ b/configs/pine64_plus_defconfig
> @@ -3,9 +3,14 @@ CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER=y
>  CONFIG_ARCH_SUNXI=y
>  CONFIG_MACH_SUN50I=y
>  CONFIG_DEFAULT_DEVICE_TREE="sun50i-a64-pine64-plus"
> +CONFIG_OF_LIST="sun50i-a64-pine64 sun50i-a64-pine64-plus"
>  # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
>  CONFIG_CONSOLE_MUX=y
>  CONFIG_SPL=y
> +CONFIG_FIT=y
> +CONFIG_SPL_FIT=y
> +CONFIG_SPL_LOAD_FIT=y
> +CONFIG_SPL_OF_LIBFDT=y

I see no reason why this shouldn't be enabled by default through
Kconfig, those are pretty standard usecases.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170120/52de5b8d/attachment.sig>

[U-Boot] [RFC PATCH 09/11] sunxi: Pine64: defconfig: enable SPL FIT support

[André Przywara]

On 20/01/17 21:36, Maxime Ripard wrote:
> On Fri, Jan 20, 2017 at 01:53:29AM +0000, Andre Przywara wrote:
>> The Pine64 (as all 64-bit Allwinner boards so far) need to load an
>> ARM Trusted Firmware image beside the actual U-Boot proper.
>> This can now be easily achieved by using the just extended SPL FIT
>> loading support, so enable it in the Pine64 defconfig.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  configs/pine64_plus_defconfig | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/configs/pine64_plus_defconfig b/configs/pine64_plus_defconfig
>> index 2374170..e050800 100644
>> --- a/configs/pine64_plus_defconfig
>> +++ b/configs/pine64_plus_defconfig
>> @@ -3,9 +3,14 @@ CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER=y
>>  CONFIG_ARCH_SUNXI=y
>>  CONFIG_MACH_SUN50I=y
>>  CONFIG_DEFAULT_DEVICE_TREE="sun50i-a64-pine64-plus"
>> +CONFIG_OF_LIST="sun50i-a64-pine64 sun50i-a64-pine64-plus"
>>  # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
>>  CONFIG_CONSOLE_MUX=y
>>  CONFIG_SPL=y
>> +CONFIG_FIT=y
>> +CONFIG_SPL_FIT=y
>> +CONFIG_SPL_LOAD_FIT=y
>> +CONFIG_SPL_OF_LIBFDT=y
> 
> I see no reason why this shouldn't be enabled by default through
> Kconfig, those are pretty standard usecases.

Ah, indeed.
I didn't dare ;-)

Cheers,
Andre.

[U-Boot] [RFC PATCH 10/11] sunxi: Pine64: add FIT image source

[Andre Przywara]

The Pine64 boards require an ARM Trusted Firmware (ATF) image to be
loaded and executes prior to the actual U-Boot proper.
Add a FIT image source file to describe the binaries, also add the
supported DTs to be able to boot multiple boards with one image. Use:
$ tools/mkimage -f boards/sunxi/pine64_atf.its -E pine64_image.itb
to create the image file. Copy (or symlink) the bl31.bin file from
the ARM Trusted Firmware build into the current directory.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 board/sunxi/pine64_atf.its | 54 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 board/sunxi/pine64_atf.its

diff --git a/board/sunxi/pine64_atf.its b/board/sunxi/pine64_atf.its
new file mode 100644
index 0000000..53fce87
--- /dev/null
+++ b/board/sunxi/pine64_atf.its
@@ -0,0 +1,54 @@
+/dts-v1/;
+
+/ {
+	description = "Configuration to load ATF before U-Boot";
+	#address-cells = <1>;
+
+	images {
+		uboot at 1 {
+			description = "U-Boot (64-bit)";
+			data = /incbin/("../../u-boot-nodtb.bin");
+			type = "standalone";
+			arch = "arm64";
+			compression = "none";
+			load = <0x4a000000>;
+		};
+		atf at 1 {
+			description = "ARM Trusted Firmware";
+			data = /incbin/("../../bl31.bin");
+			type = "firmware";
+			arch = "arm64";
+			compression = "none";
+			load = <0x44000>;
+			entry = <0x44000>;
+		};
+		fdt at 1 {
+			description = "Pine64+ DT";
+			data = /incbin/("../../arch/arm/dts/sun50i-a64-pine64-plus.dtb");
+                        type = "flat_dt";
+			compression = "none";
+		};
+		fdt at 2 {
+			description = "Pine64 DT";
+			data = /incbin/("../../arch/arm/dts/sun50i-a64-pine64.dtb");
+                        type = "flat_dt";
+			compression = "none";
+		};
+	};
+	configurations {
+		default = "config at 1";
+
+		config at 1 {
+			description = "sun50i-a64-pine64-plus";
+			firmware = "uboot at 1";
+			loadables = "atf at 1";
+			fdt = "fdt at 1";
+		};
+		config at 2 {
+			description = "sun50i-a64-pine64";
+			firmware = "uboot at 1";
+			loadables = "atf at 1";
+			fdt = "fdt at 2";
+		};
+	};
+};
-- 
2.8.2

[U-Boot] [RFC PATCH 11/11] SPL: SPI: sunxi: add SPL FIT image support

[Andre Przywara]

The sunxi-specific SPI load routine only knows how to load a legacy
U-Boot image.
Teach it how to handle FIT images as well, simply by providing the
existing SPL FIT loader with the right loader routine to access the SPI
NOR flash.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/mtd/spi/sunxi_spi_spl.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/spi/sunxi_spi_spl.c b/drivers/mtd/spi/sunxi_spi_spl.c
index a24c115..187b39a 100644
--- a/drivers/mtd/spi/sunxi_spi_spl.c
+++ b/drivers/mtd/spi/sunxi_spi_spl.c
@@ -8,6 +8,7 @@
 #include <spl.h>
 #include <asm/gpio.h>
 #include <asm/io.h>
+#include <libfdt.h>
 
 #ifdef CONFIG_SPL_OS_BOOT
 #error CONFIG_SPL_OS_BOOT is not supported yet
@@ -261,27 +262,51 @@ static void spi0_read_data(void *buf, u32 addr, u32 len)
 	}
 }
 
+static ulong spi_load_read(struct spl_load_info *load, ulong sector,
+			   ulong count, void *buf)
+{
+	spi0_read_data(buf, sector, count);
+
+	return count;
+}
+
 /*****************************************************************************/
 
 static int spl_spi_load_image(struct spl_image_info *spl_image,
 			      struct spl_boot_device *bootdev)
 {
-	int err;
+	int ret = 0;
 	struct image_header *header;
 	header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
 
 	spi0_init();
 
 	spi0_read_data((void *)header, CONFIG_SYS_SPI_U_BOOT_OFFS, 0x40);
-	err = spl_parse_image_header(spl_image, header);
-	if (err)
-		return err;
 
-	spi0_read_data((void *)spl_image->load_addr, CONFIG_SYS_SPI_U_BOOT_OFFS,
-		       spl_image->size);
+        if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
+		image_get_magic(header) == FDT_MAGIC) {
+		struct spl_load_info load;
+
+		debug("Found FIT image\n");
+		load.dev = NULL;
+		load.priv = NULL;
+		load.filename = NULL;
+		load.bl_len = 1;
+		load.read = spi_load_read;
+		ret = spl_load_simple_fit(spl_image, &load,
+					  CONFIG_SYS_SPI_U_BOOT_OFFS, header);
+	} else {
+		ret = spl_parse_image_header(spl_image, header);
+		if (ret)
+			return ret;
+
+		spi0_read_data((void *)spl_image->load_addr,
+			       CONFIG_SYS_SPI_U_BOOT_OFFS, spl_image->size);
+	}
 
 	spi0_deinit();
-	return 0;
+
+	return ret;
 }
 /* Use priorty 0 to override the default if it happens to be linked in */
 SPL_LOAD_IMAGE_METHOD("sunxi SPI", 0, BOOT_DEVICE_SPI, spl_spi_load_image);
-- 
2.8.2

[U-Boot] [RFC PATCH 11/11] SPL: SPI: sunxi: add SPL FIT image support

[Maxime Ripard]

Hi,

On Fri, Jan 20, 2017 at 01:53:31AM +0000, Andre Przywara wrote:
> The sunxi-specific SPI load routine only knows how to load a legacy
> U-Boot image.
> Teach it how to handle FIT images as well, simply by providing the
> existing SPL FIT loader with the right loader routine to access the SPI
> NOR flash.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Does the MMC driver need a similar patch?

Thanks
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170120/bd3b9e31/attachment.sig>

[U-Boot] [RFC PATCH 11/11] SPL: SPI: sunxi: add SPL FIT image support

[André Przywara]

On 20/01/17 21:37, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Jan 20, 2017 at 01:53:31AM +0000, Andre Przywara wrote:
>> The sunxi-specific SPI load routine only knows how to load a legacy
>> U-Boot image.
>> Teach it how to handle FIT images as well, simply by providing the
>> existing SPL FIT loader with the right loader routine to access the SPI
>> NOR flash.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> Does the MMC driver need a similar patch?

No, the other SPL boot media drivers all support FIT already. It's just
that the sunxi SPL SPI driver is a bit special and was designed to be as
small as possible, so it didn't have these few bits.
And apparently no one used FIT with it before.

Cheers,
Andre

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Andrew F. Davis]

On 01/19/2017 07:53 PM, Andre Przywara wrote:
> Currently the FIT format is not used to its full potential in the SPL:
> It only loads the first image from the /images node and appends the
> proper FDT.
> Some boards and platforms would benefit from loading more images before
> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
> 
> This series tries to solve this in a board agnostic and generic way:
> We extend the SPL FIT loading scheme to allow loading multiple images.
> So apart from loading the image which is referenced by the "firmware"
> property in the respective configuration node and placing the DTB right
> behind it, we iterate over all strings in the "loadable" property.
> Each image referenced there will be loaded to its specified load address.
> The entry point U-Boot eventually branches to will be taken from the
> first image to explicitly provide the "entry" property, or, if none
> of them does so, from the load address of the "firmware" image.
> This keeps the scheme compatible with the FIT images our Makefile creates
> automatically at the moment.
> 
> Apart from the already mentioned ATF scenario this opens up more usage
> scenarios, of which the commit message of patch 04/11 lists some.
> 

I have been thinking about a similar problem we are facing regarding
OP-TEE loading when doing SPL-only boots. I think extending SPL FIT
loader is a good approach, I've just been concerned about SPL bloat. On
our High Security enabled AM335x SoC we only have 41KB of SRAM to load
SPL (last I checked we only have 100 bytes of overhead left), and we
need FIT support as we use it for image authentication (it's what we use
the board_fit_image_post_process() hook for), so any changes to SPL FIT
need to be carefully made.

> The first three patches rework the SPL FIT support to be more flexible
> and to allow easier usage in the fourth patch, which introduces the
> multiple-image loading facility.
> The remaining patches enable that support for the Pine64 board to make
> its SPL support finally useful and to demonstrate usage of this scheme:
> patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
> compilation of the SPL with FIT support enabled. Patch 8 implements the
> board selector routine, which selects either the Pine64 or Pine64+ DTB
> depending on the detected DRAM size. Patch 9 enables SPL FIT support in
> the Pine64 defconfig.
> To demonstrate the usage, patch 10 provides a FIT source file, which
> loads and executes ATF before the U-Boot proper. Users are expected to
> compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
> then write the resulting file behind the SPL on an SD card (or any other
> U-Boot supported boot media, for that matter).
> Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
> which allows to load ATF on boards with SPI flash as well.
> 
> Questions:
> 1) Is this scheme the right one (usage of "firmware" and "loadables",
>    determination of entry point)? Shall we make use of the "setup"
>    property?
> 2) Shall we extend mkimage to allow supplying "loadable" files on the
>    command line, which would allow to build the .itb file automatically?

Where does this end, we may also need to add the load address for
images, etc... eventually we are passing a full .its on the command
line. I'm not against this, as it would help our build process also (we
generate an .its file with all our build artifacts during our distro
build and feed it to mkimage), so if we think this is better than option
3 below, we should think through all the things we may need before
cluttering up the mkimage command line arguments.

Andrew

> 3) Is providing the .its source file for a (family of) boards the right
>    way?
> 4) Does this break any boards which already use SPL FIT loading?
> 
> And for the Pine64 part:
> 5) Is extending the usable SPL size like in patch 5-7 acceptable?
> 
> I have a more generic solution for the .dtb selection in mind: Based on
> some patch from Siarhei we store the .dtb filename in the SPL header and
> select the .dtb from the FIT image by simply matching the name. This would
> allow _one_ build supporting multiple boards. The actual board name would
> need to written into the SPL header or could be copied from there when
> updating the image.
> I can provide the patches once we agreed upon this series.
> 
> Please let me know what you think!
> 
> Cheers,
> Andre.
> 
> Andre Przywara (11):
>   SPL: FIT: refactor FDT loading
>   SPL: FIT: rework U-Boot image loading
>   SPL: FIT: factor out spl_load_fit_image()
>   SPL: FIT: allow loading multiple images
>   tools: mksunxiboot: allow larger SPL binaries
>   sunxi: A64: SPL: allow large SPL binary
>   sunxi: A64: move SPL stack to end of SRAM A2
>   sunxi: SPL: add FIT config selector for Pine64 boards
>   sunxi: Pine64: defconfig: enable SPL FIT support
>   sunxi: Pine64: add FIT image source
>   SPL: SPI: sunxi: add SPL FIT image support
> 
>  board/sunxi/board.c             |  13 +++
>  board/sunxi/pine64_atf.its      |  54 +++++++++
>  common/spl/spl_fit.c            | 241 +++++++++++++++++++++++-----------------
>  configs/pine64_plus_defconfig   |   5 +
>  drivers/mtd/spi/sunxi_spi_spl.c |  39 +++++--
>  include/configs/sunxi-common.h  |   6 +-
>  scripts/Makefile.spl            |   7 +-
>  tools/mksunxiboot.c             |  51 ++++++---
>  8 files changed, 290 insertions(+), 126 deletions(-)
>  create mode 100644 board/sunxi/pine64_atf.its
> 

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Andre Przywara]

Hi Andrew,

thanks for the comments.

On 20/01/17 17:02, Andrew F. Davis wrote:
> On 01/19/2017 07:53 PM, Andre Przywara wrote:
>> Currently the FIT format is not used to its full potential in the SPL:
>> It only loads the first image from the /images node and appends the
>> proper FDT.
>> Some boards and platforms would benefit from loading more images before
>> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
>> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>>
>> This series tries to solve this in a board agnostic and generic way:
>> We extend the SPL FIT loading scheme to allow loading multiple images.
>> So apart from loading the image which is referenced by the "firmware"
>> property in the respective configuration node and placing the DTB right
>> behind it, we iterate over all strings in the "loadable" property.
>> Each image referenced there will be loaded to its specified load address.
>> The entry point U-Boot eventually branches to will be taken from the
>> first image to explicitly provide the "entry" property, or, if none
>> of them does so, from the load address of the "firmware" image.
>> This keeps the scheme compatible with the FIT images our Makefile creates
>> automatically at the moment.
>>
>> Apart from the already mentioned ATF scenario this opens up more usage
>> scenarios, of which the commit message of patch 04/11 lists some.
>>
> 
> I have been thinking about a similar problem we are facing regarding
> OP-TEE loading when doing SPL-only boots. I think extending SPL FIT
> loader is a good approach, I've just been concerned about SPL bloat. On
> our High Security enabled AM335x SoC we only have 41KB of SRAM to load
> SPL (last I checked we only have 100 bytes of overhead left), and we
> need FIT support as we use it for image authentication (it's what we use
> the board_fit_image_post_process() hook for), so any changes to SPL FIT
> need to be carefully made.

Understood. Have you actually included FIT support as it exists already
in your build? My observation is that even without these changes
proposed here SPL FIT adds quite a bit to the SPL size (hence my patches
to extend SPL size for AArch64 sunxi). This is mostly due to some libfdt
bits being included, so I guess there is not much you can do about it. I
will later check how much the code size changes with my patches.
And btw.: Allwinner has either 24KB or 32KB of SRAM, so the situation is
even more dire here.

>> The first three patches rework the SPL FIT support to be more flexible
>> and to allow easier usage in the fourth patch, which introduces the
>> multiple-image loading facility.
>> The remaining patches enable that support for the Pine64 board to make
>> its SPL support finally useful and to demonstrate usage of this scheme:
>> patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
>> compilation of the SPL with FIT support enabled. Patch 8 implements the
>> board selector routine, which selects either the Pine64 or Pine64+ DTB
>> depending on the detected DRAM size. Patch 9 enables SPL FIT support in
>> the Pine64 defconfig.
>> To demonstrate the usage, patch 10 provides a FIT source file, which
>> loads and executes ATF before the U-Boot proper. Users are expected to
>> compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
>> then write the resulting file behind the SPL on an SD card (or any other
>> U-Boot supported boot media, for that matter).
>> Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
>> which allows to load ATF on boards with SPI flash as well.
>>
>> Questions:
>> 1) Is this scheme the right one (usage of "firmware" and "loadables",
>>    determination of entry point)? Shall we make use of the "setup"
>>    property?
>> 2) Shall we extend mkimage to allow supplying "loadable" files on the
>>    command line, which would allow to build the .itb file automatically?
> 
> Where does this end, we may also need to add the load address for
> images, etc... eventually we are passing a full .its on the command
> line. I'm not against this, as it would help our build process also (we
> generate an .its file with all our build artifacts during our distro
> build and feed it to mkimage), so if we think this is better than option
> 3 below, we should think through all the things we may need before
> cluttering up the mkimage command line arguments.

I agree, I was wondering about that as well. It's just tempting to add
one or two options and piggy-back on the existing Makefile support.

Maybe some generic infrastructure of storing platform or board specific
.its files can be included into the build system? Similarly to .dts
files we could reference them in the (def)config and use mkimage -f
during build to generate an image? That sounds more flexible than adding
command line options to cover specific scenarios. We may use something
like mergeconfig to make changes to the .its when needed.

Cheers,
Andre.

>> 3) Is providing the .its source file for a (family of) boards the right
>>    way?
>> 4) Does this break any boards which already use SPL FIT loading?
>>
>> And for the Pine64 part:
>> 5) Is extending the usable SPL size like in patch 5-7 acceptable?
>>
>> I have a more generic solution for the .dtb selection in mind: Based on
>> some patch from Siarhei we store the .dtb filename in the SPL header and
>> select the .dtb from the FIT image by simply matching the name. This would
>> allow _one_ build supporting multiple boards. The actual board name would
>> need to written into the SPL header or could be copied from there when
>> updating the image.
>> I can provide the patches once we agreed upon this series.
>>
>> Please let me know what you think!
>>
>> Cheers,
>> Andre.
>>
>> Andre Przywara (11):
>>   SPL: FIT: refactor FDT loading
>>   SPL: FIT: rework U-Boot image loading
>>   SPL: FIT: factor out spl_load_fit_image()
>>   SPL: FIT: allow loading multiple images
>>   tools: mksunxiboot: allow larger SPL binaries
>>   sunxi: A64: SPL: allow large SPL binary
>>   sunxi: A64: move SPL stack to end of SRAM A2
>>   sunxi: SPL: add FIT config selector for Pine64 boards
>>   sunxi: Pine64: defconfig: enable SPL FIT support
>>   sunxi: Pine64: add FIT image source
>>   SPL: SPI: sunxi: add SPL FIT image support
>>
>>  board/sunxi/board.c             |  13 +++
>>  board/sunxi/pine64_atf.its      |  54 +++++++++
>>  common/spl/spl_fit.c            | 241 +++++++++++++++++++++++-----------------
>>  configs/pine64_plus_defconfig   |   5 +
>>  drivers/mtd/spi/sunxi_spi_spl.c |  39 +++++--
>>  include/configs/sunxi-common.h  |   6 +-
>>  scripts/Makefile.spl            |   7 +-
>>  tools/mksunxiboot.c             |  51 ++++++---
>>  8 files changed, 290 insertions(+), 126 deletions(-)
>>  create mode 100644 board/sunxi/pine64_atf.its
>>

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Andrew F. Davis]

On 01/20/2017 11:17 AM, Andre Przywara wrote:
> Hi Andrew,
> 
> thanks for the comments.
> 
> On 20/01/17 17:02, Andrew F. Davis wrote:
>> On 01/19/2017 07:53 PM, Andre Przywara wrote:
>>> Currently the FIT format is not used to its full potential in the SPL:
>>> It only loads the first image from the /images node and appends the
>>> proper FDT.
>>> Some boards and platforms would benefit from loading more images before
>>> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
>>> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>>>
>>> This series tries to solve this in a board agnostic and generic way:
>>> We extend the SPL FIT loading scheme to allow loading multiple images.
>>> So apart from loading the image which is referenced by the "firmware"
>>> property in the respective configuration node and placing the DTB right
>>> behind it, we iterate over all strings in the "loadable" property.
>>> Each image referenced there will be loaded to its specified load address.
>>> The entry point U-Boot eventually branches to will be taken from the
>>> first image to explicitly provide the "entry" property, or, if none
>>> of them does so, from the load address of the "firmware" image.
>>> This keeps the scheme compatible with the FIT images our Makefile creates
>>> automatically at the moment.
>>>
>>> Apart from the already mentioned ATF scenario this opens up more usage
>>> scenarios, of which the commit message of patch 04/11 lists some.
>>>
>>
>> I have been thinking about a similar problem we are facing regarding
>> OP-TEE loading when doing SPL-only boots. I think extending SPL FIT
>> loader is a good approach, I've just been concerned about SPL bloat. On
>> our High Security enabled AM335x SoC we only have 41KB of SRAM to load
>> SPL (last I checked we only have 100 bytes of overhead left), and we
>> need FIT support as we use it for image authentication (it's what we use
>> the board_fit_image_post_process() hook for), so any changes to SPL FIT
>> need to be carefully made.
> 
> Understood. Have you actually included FIT support as it exists already
> in your build? My observation is that even without these changes
> proposed here SPL FIT adds quite a bit to the SPL size (hence my patches
> to extend SPL size for AArch64 sunxi). This is mostly due to some libfdt
> bits being included, so I guess there is not much you can do about it. I
> will later check how much the code size changes with my patches.
> And btw.: Allwinner has either 24KB or 32KB of SRAM, so the situation is
> even more dire here.
> 

Yes, we include FIT support in our build, and we make a lot of
sacrifices in functionality for it (reducing the types of media we can
load from, tiny malloc, tiny printf, etc..).

When new features come out we need to decide which old ones to disable,
so the benefits of FIT and other features have to be worth more than the
size in code it takes to implement them. (I think this change is worth
it and am willing to sacrifice other things to keep our boards booting
and have this addition, but we need to try to keep this in size issue in
mind).

>>> The first three patches rework the SPL FIT support to be more flexible
>>> and to allow easier usage in the fourth patch, which introduces the
>>> multiple-image loading facility.
>>> The remaining patches enable that support for the Pine64 board to make
>>> its SPL support finally useful and to demonstrate usage of this scheme:
>>> patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
>>> compilation of the SPL with FIT support enabled. Patch 8 implements the
>>> board selector routine, which selects either the Pine64 or Pine64+ DTB
>>> depending on the detected DRAM size. Patch 9 enables SPL FIT support in
>>> the Pine64 defconfig.
>>> To demonstrate the usage, patch 10 provides a FIT source file, which
>>> loads and executes ATF before the U-Boot proper. Users are expected to
>>> compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
>>> then write the resulting file behind the SPL on an SD card (or any other
>>> U-Boot supported boot media, for that matter).
>>> Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
>>> which allows to load ATF on boards with SPI flash as well.
>>>
>>> Questions:
>>> 1) Is this scheme the right one (usage of "firmware" and "loadables",
>>>    determination of entry point)? Shall we make use of the "setup"
>>>    property?
>>> 2) Shall we extend mkimage to allow supplying "loadable" files on the
>>>    command line, which would allow to build the .itb file automatically?
>>
>> Where does this end, we may also need to add the load address for
>> images, etc... eventually we are passing a full .its on the command
>> line. I'm not against this, as it would help our build process also (we
>> generate an .its file with all our build artifacts during our distro
>> build and feed it to mkimage), so if we think this is better than option
>> 3 below, we should think through all the things we may need before
>> cluttering up the mkimage command line arguments.
> 
> I agree, I was wondering about that as well. It's just tempting to add
> one or two options and piggy-back on the existing Makefile support.
> 
> Maybe some generic infrastructure of storing platform or board specific
> .its files can be included into the build system? Similarly to .dts
> files we could reference them in the (def)config and use mkimage -f
> during build to generate an image? That sounds more flexible than adding
> command line options to cover specific scenarios. We may use something
> like mergeconfig to make changes to the .its when needed.
> 

I'm okay with that, as these .its files are .dts based anyway it
shouldn't be too hard to have #includes to get other image specific
nodes and overrides. One thing I'm not sure about is having the .its
files stored in U-Boot tree, as most of the stuff we add to our .itb
loadable section is external (ATF, OP-TEE, IPU/IVA firmware, etc..), but
I'm all for adding the support to mkimage for such a thing anyway.

Andrew

> Cheers,
> Andre.
> 
>>> 3) Is providing the .its source file for a (family of) boards the right
>>>    way?
>>> 4) Does this break any boards which already use SPL FIT loading?
>>>
>>> And for the Pine64 part:
>>> 5) Is extending the usable SPL size like in patch 5-7 acceptable?
>>>
>>> I have a more generic solution for the .dtb selection in mind: Based on
>>> some patch from Siarhei we store the .dtb filename in the SPL header and
>>> select the .dtb from the FIT image by simply matching the name. This would
>>> allow _one_ build supporting multiple boards. The actual board name would
>>> need to written into the SPL header or could be copied from there when
>>> updating the image.
>>> I can provide the patches once we agreed upon this series.
>>>
>>> Please let me know what you think!
>>>
>>> Cheers,
>>> Andre.
>>>
>>> Andre Przywara (11):
>>>   SPL: FIT: refactor FDT loading
>>>   SPL: FIT: rework U-Boot image loading
>>>   SPL: FIT: factor out spl_load_fit_image()
>>>   SPL: FIT: allow loading multiple images
>>>   tools: mksunxiboot: allow larger SPL binaries
>>>   sunxi: A64: SPL: allow large SPL binary
>>>   sunxi: A64: move SPL stack to end of SRAM A2
>>>   sunxi: SPL: add FIT config selector for Pine64 boards
>>>   sunxi: Pine64: defconfig: enable SPL FIT support
>>>   sunxi: Pine64: add FIT image source
>>>   SPL: SPI: sunxi: add SPL FIT image support
>>>
>>>  board/sunxi/board.c             |  13 +++
>>>  board/sunxi/pine64_atf.its      |  54 +++++++++
>>>  common/spl/spl_fit.c            | 241 +++++++++++++++++++++++-----------------
>>>  configs/pine64_plus_defconfig   |   5 +
>>>  drivers/mtd/spi/sunxi_spi_spl.c |  39 +++++--
>>>  include/configs/sunxi-common.h  |   6 +-
>>>  scripts/Makefile.spl            |   7 +-
>>>  tools/mksunxiboot.c             |  51 ++++++---
>>>  8 files changed, 290 insertions(+), 126 deletions(-)
>>>  create mode 100644 board/sunxi/pine64_atf.its
>>>

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Andre Przywara]

Hi,

On 20/01/17 17:35, Andrew F. Davis wrote:
> On 01/20/2017 11:17 AM, Andre Przywara wrote:
>> Hi Andrew,
>>
>> thanks for the comments.
>>
>> On 20/01/17 17:02, Andrew F. Davis wrote:
>>> On 01/19/2017 07:53 PM, Andre Przywara wrote:
>>>> Currently the FIT format is not used to its full potential in the SPL:
>>>> It only loads the first image from the /images node and appends the
>>>> proper FDT.
>>>> Some boards and platforms would benefit from loading more images before
>>>> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
>>>> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>>>>
>>>> This series tries to solve this in a board agnostic and generic way:
>>>> We extend the SPL FIT loading scheme to allow loading multiple images.
>>>> So apart from loading the image which is referenced by the "firmware"
>>>> property in the respective configuration node and placing the DTB right
>>>> behind it, we iterate over all strings in the "loadable" property.
>>>> Each image referenced there will be loaded to its specified load address.
>>>> The entry point U-Boot eventually branches to will be taken from the
>>>> first image to explicitly provide the "entry" property, or, if none
>>>> of them does so, from the load address of the "firmware" image.
>>>> This keeps the scheme compatible with the FIT images our Makefile creates
>>>> automatically at the moment.
>>>>
>>>> Apart from the already mentioned ATF scenario this opens up more usage
>>>> scenarios, of which the commit message of patch 04/11 lists some.
>>>>
>>>
>>> I have been thinking about a similar problem we are facing regarding
>>> OP-TEE loading when doing SPL-only boots. I think extending SPL FIT
>>> loader is a good approach, I've just been concerned about SPL bloat. On
>>> our High Security enabled AM335x SoC we only have 41KB of SRAM to load
>>> SPL (last I checked we only have 100 bytes of overhead left), and we
>>> need FIT support as we use it for image authentication (it's what we use
>>> the board_fit_image_post_process() hook for), so any changes to SPL FIT
>>> need to be carefully made.
>>
>> Understood. Have you actually included FIT support as it exists already
>> in your build? My observation is that even without these changes
>> proposed here SPL FIT adds quite a bit to the SPL size (hence my patches
>> to extend SPL size for AArch64 sunxi). This is mostly due to some libfdt
>> bits being included, so I guess there is not much you can do about it. I
>> will later check how much the code size changes with my patches.
>> And btw.: Allwinner has either 24KB or 32KB of SRAM, so the situation is
>> even more dire here.
>>
> 
> Yes, we include FIT support in our build, and we make a lot of
> sacrifices in functionality for it (reducing the types of media we can
> load from, tiny malloc, tiny printf, etc..).
> 
> When new features come out we need to decide which old ones to disable,
> so the benefits of FIT and other features have to be worth more than the
> size in code it takes to implement them. (I think this change is worth
> it and am willing to sacrifice other things to keep our boards booting
> and have this addition, but we need to try to keep this in size issue in
> mind).

OK, got it. I have some easy patches to cut off 250 Bytes or so from an
AArch64 build, maybe there is something similar possible for ARM?
For instance I converted the ctype() implementation from table based to
using simple comparisons and could cover all cases needed by the SPL and
save 200 Bytes. After a quick look at am335x_shc_sdboot_defconfig it
seems you have the 256 Byte ctype() table in there, so I can send this
patch to give you more back than this FIT extension takes ;-)

>>>> The first three patches rework the SPL FIT support to be more flexible
>>>> and to allow easier usage in the fourth patch, which introduces the
>>>> multiple-image loading facility.
>>>> The remaining patches enable that support for the Pine64 board to make
>>>> its SPL support finally useful and to demonstrate usage of this scheme:
>>>> patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
>>>> compilation of the SPL with FIT support enabled. Patch 8 implements the
>>>> board selector routine, which selects either the Pine64 or Pine64+ DTB
>>>> depending on the detected DRAM size. Patch 9 enables SPL FIT support in
>>>> the Pine64 defconfig.
>>>> To demonstrate the usage, patch 10 provides a FIT source file, which
>>>> loads and executes ATF before the U-Boot proper. Users are expected to
>>>> compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
>>>> then write the resulting file behind the SPL on an SD card (or any other
>>>> U-Boot supported boot media, for that matter).
>>>> Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
>>>> which allows to load ATF on boards with SPI flash as well.
>>>>
>>>> Questions:
>>>> 1) Is this scheme the right one (usage of "firmware" and "loadables",
>>>>    determination of entry point)? Shall we make use of the "setup"
>>>>    property?
>>>> 2) Shall we extend mkimage to allow supplying "loadable" files on the
>>>>    command line, which would allow to build the .itb file automatically?
>>>
>>> Where does this end, we may also need to add the load address for
>>> images, etc... eventually we are passing a full .its on the command
>>> line. I'm not against this, as it would help our build process also (we
>>> generate an .its file with all our build artifacts during our distro
>>> build and feed it to mkimage), so if we think this is better than option
>>> 3 below, we should think through all the things we may need before
>>> cluttering up the mkimage command line arguments.
>>
>> I agree, I was wondering about that as well. It's just tempting to add
>> one or two options and piggy-back on the existing Makefile support.
>>
>> Maybe some generic infrastructure of storing platform or board specific
>> .its files can be included into the build system? Similarly to .dts
>> files we could reference them in the (def)config and use mkimage -f
>> during build to generate an image? That sounds more flexible than adding
>> command line options to cover specific scenarios. We may use something
>> like mergeconfig to make changes to the .its when needed.
>>
> 
> I'm okay with that, as these .its files are .dts based anyway it
> shouldn't be too hard to have #includes to get other image specific
> nodes and overrides. One thing I'm not sure about is having the .its
> files stored in U-Boot tree, as most of the stuff we add to our .itb
> loadable section is external (ATF, OP-TEE, IPU/IVA firmware, etc..), but
> I'm all for adding the support to mkimage for such a thing anyway.

Yeah, that's an interesting point. Allwinner also needs the ATF bl31.bin
to be provided somehow. In the "demo" .its I put it into U-Boot root,
maybe we should create a directory "externals" or the like with just a
README in it, and people are supposed to copy or link in the binary
images there? In the .its it would read "../../externals/...", for
instance, to make it more clear that those components are external to
and not provided by U-Boot.

Cheers,
Andre.

>> Cheers,
>> Andre.
>>
>>>> 3) Is providing the .its source file for a (family of) boards the right
>>>>    way?
>>>> 4) Does this break any boards which already use SPL FIT loading?
>>>>
>>>> And for the Pine64 part:
>>>> 5) Is extending the usable SPL size like in patch 5-7 acceptable?
>>>>
>>>> I have a more generic solution for the .dtb selection in mind: Based on
>>>> some patch from Siarhei we store the .dtb filename in the SPL header and
>>>> select the .dtb from the FIT image by simply matching the name. This would
>>>> allow _one_ build supporting multiple boards. The actual board name would
>>>> need to written into the SPL header or could be copied from there when
>>>> updating the image.
>>>> I can provide the patches once we agreed upon this series.
>>>>
>>>> Please let me know what you think!
>>>>
>>>> Cheers,
>>>> Andre.
>>>>
>>>> Andre Przywara (11):
>>>>   SPL: FIT: refactor FDT loading
>>>>   SPL: FIT: rework U-Boot image loading
>>>>   SPL: FIT: factor out spl_load_fit_image()
>>>>   SPL: FIT: allow loading multiple images
>>>>   tools: mksunxiboot: allow larger SPL binaries
>>>>   sunxi: A64: SPL: allow large SPL binary
>>>>   sunxi: A64: move SPL stack to end of SRAM A2
>>>>   sunxi: SPL: add FIT config selector for Pine64 boards
>>>>   sunxi: Pine64: defconfig: enable SPL FIT support
>>>>   sunxi: Pine64: add FIT image source
>>>>   SPL: SPI: sunxi: add SPL FIT image support
>>>>
>>>>  board/sunxi/board.c             |  13 +++
>>>>  board/sunxi/pine64_atf.its      |  54 +++++++++
>>>>  common/spl/spl_fit.c            | 241 +++++++++++++++++++++++-----------------
>>>>  configs/pine64_plus_defconfig   |   5 +
>>>>  drivers/mtd/spi/sunxi_spi_spl.c |  39 +++++--
>>>>  include/configs/sunxi-common.h  |   6 +-
>>>>  scripts/Makefile.spl            |   7 +-
>>>>  tools/mksunxiboot.c             |  51 ++++++---
>>>>  8 files changed, 290 insertions(+), 126 deletions(-)
>>>>  create mode 100644 board/sunxi/pine64_atf.its
>>>>

[U-Boot] [linux-sunxi] Re: [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Icenowy Zheng]

21.01.2017, 01:47, "Andre Przywara" <andre.przywara@arm.com>:
> Hi,
>
> On 20/01/17 17:35, Andrew F. Davis wrote:
>> ?On 01/20/2017 11:17 AM, Andre Przywara wrote:
>>> ?Hi Andrew,
>>>
>>> ?thanks for the comments.
>>>
>>> ?On 20/01/17 17:02, Andrew F. Davis wrote:
>>>> ?On 01/19/2017 07:53 PM, Andre Przywara wrote:
>>>>> ?Currently the FIT format is not used to its full potential in the SPL:
>>>>> ?It only loads the first image from the /images node and appends the
>>>>> ?proper FDT.
>>>>> ?Some boards and platforms would benefit from loading more images before
>>>>> ?starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
>>>>> ?which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>>>>>
>>>>> ?This series tries to solve this in a board agnostic and generic way:
>>>>> ?We extend the SPL FIT loading scheme to allow loading multiple images.
>>>>> ?So apart from loading the image which is referenced by the "firmware"
>>>>> ?property in the respective configuration node and placing the DTB right
>>>>> ?behind it, we iterate over all strings in the "loadable" property.
>>>>> ?Each image referenced there will be loaded to its specified load address.
>>>>> ?The entry point U-Boot eventually branches to will be taken from the
>>>>> ?first image to explicitly provide the "entry" property, or, if none
>>>>> ?of them does so, from the load address of the "firmware" image.
>>>>> ?This keeps the scheme compatible with the FIT images our Makefile creates
>>>>> ?automatically at the moment.
>>>>>
>>>>> ?Apart from the already mentioned ATF scenario this opens up more usage
>>>>> ?scenarios, of which the commit message of patch 04/11 lists some.
>>>>
>>>> ?I have been thinking about a similar problem we are facing regarding
>>>> ?OP-TEE loading when doing SPL-only boots. I think extending SPL FIT
>>>> ?loader is a good approach, I've just been concerned about SPL bloat. On
>>>> ?our High Security enabled AM335x SoC we only have 41KB of SRAM to load
>>>> ?SPL (last I checked we only have 100 bytes of overhead left), and we
>>>> ?need FIT support as we use it for image authentication (it's what we use
>>>> ?the board_fit_image_post_process() hook for), so any changes to SPL FIT
>>>> ?need to be carefully made.
>>>
>>> ?Understood. Have you actually included FIT support as it exists already
>>> ?in your build? My observation is that even without these changes
>>> ?proposed here SPL FIT adds quite a bit to the SPL size (hence my patches
>>> ?to extend SPL size for AArch64 sunxi). This is mostly due to some libfdt
>>> ?bits being included, so I guess there is not much you can do about it. I
>>> ?will later check how much the code size changes with my patches.
>>> ?And btw.: Allwinner has either 24KB or 32KB of SRAM, so the situation is
>>> ?even more dire here.
>>
>> ?Yes, we include FIT support in our build, and we make a lot of
>> ?sacrifices in functionality for it (reducing the types of media we can
>> ?load from, tiny malloc, tiny printf, etc..).
>>
>> ?When new features come out we need to decide which old ones to disable,
>> ?so the benefits of FIT and other features have to be worth more than the
>> ?size in code it takes to implement them. (I think this change is worth
>> ?it and am willing to sacrifice other things to keep our boards booting
>> ?and have this addition, but we need to try to keep this in size issue in
>> ?mind).
>
> OK, got it. I have some easy patches to cut off 250 Bytes or so from an
> AArch64 build, maybe there is something similar possible for ARM?
> For instance I converted the ctype() implementation from table based to
> using simple comparisons and could cover all cases needed by the SPL and
> save 200 Bytes. After a quick look at am335x_shc_sdboot_defconfig it
> seems you have the 256 Byte ctype() table in there, so I can send this
> patch to give you more back than this FIT extension takes ;-)
>
>>>>> ?The first three patches rework the SPL FIT support to be more flexible
>>>>> ?and to allow easier usage in the fourth patch, which introduces the
>>>>> ?multiple-image loading facility.
>>>>> ?The remaining patches enable that support for the Pine64 board to make
>>>>> ?its SPL support finally useful and to demonstrate usage of this scheme:
>>>>> ?patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
>>>>> ?compilation of the SPL with FIT support enabled. Patch 8 implements the
>>>>> ?board selector routine, which selects either the Pine64 or Pine64+ DTB
>>>>> ?depending on the detected DRAM size. Patch 9 enables SPL FIT support in
>>>>> ?the Pine64 defconfig.
>>>>> ?To demonstrate the usage, patch 10 provides a FIT source file, which
>>>>> ?loads and executes ATF before the U-Boot proper. Users are expected to
>>>>> ?compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
>>>>> ?then write the resulting file behind the SPL on an SD card (or any other
>>>>> ?U-Boot supported boot media, for that matter).
>>>>> ?Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
>>>>> ?which allows to load ATF on boards with SPI flash as well.
>>>>>
>>>>> ?Questions:
>>>>> ?1) Is this scheme the right one (usage of "firmware" and "loadables",
>>>>> ????determination of entry point)? Shall we make use of the "setup"
>>>>> ????property?
>>>>> ?2) Shall we extend mkimage to allow supplying "loadable" files on the
>>>>> ????command line, which would allow to build the .itb file automatically?
>>>>
>>>> ?Where does this end, we may also need to add the load address for
>>>> ?images, etc... eventually we are passing a full .its on the command
>>>> ?line. I'm not against this, as it would help our build process also (we
>>>> ?generate an .its file with all our build artifacts during our distro
>>>> ?build and feed it to mkimage), so if we think this is better than option
>>>> ?3 below, we should think through all the things we may need before
>>>> ?cluttering up the mkimage command line arguments.
>>>
>>> ?I agree, I was wondering about that as well. It's just tempting to add
>>> ?one or two options and piggy-back on the existing Makefile support.
>>>
>>> ?Maybe some generic infrastructure of storing platform or board specific
>>> ?.its files can be included into the build system? Similarly to .dts
>>> ?files we could reference them in the (def)config and use mkimage -f
>>> ?during build to generate an image? That sounds more flexible than adding
>>> ?command line options to cover specific scenarios. We may use something
>>> ?like mergeconfig to make changes to the .its when needed.
>>
>> ?I'm okay with that, as these .its files are .dts based anyway it
>> ?shouldn't be too hard to have #includes to get other image specific
>> ?nodes and overrides. One thing I'm not sure about is having the .its
>> ?files stored in U-Boot tree, as most of the stuff we add to our .itb
>> ?loadable section is external (ATF, OP-TEE, IPU/IVA firmware, etc..), but
>> ?I'm all for adding the support to mkimage for such a thing anyway.
>
> Yeah, that's an interesting point. Allwinner also needs the ATF bl31.bin
> to be provided somehow. In the "demo" .its I put it into U-Boot root,
> maybe we should create a directory "externals" or the like with just a
> README in it, and people are supposed to copy or link in the binary
> images there? In the .its it would read "../../externals/...", for
> instance, to make it more clear that those components are external to
> and not provided by U-Boot.

Maybe you can take doc/README.x86 as a reference?

X86 platform nearly all needs external blob (except QEMU :-) ) to build
a ROM image.

>
> Cheers,
> Andre.
>
>>> ?Cheers,
>>> ?Andre.
>>>
>>>>> ?3) Is providing the .its source file for a (family of) boards the right
>>>>> ????way?
>>>>> ?4) Does this break any boards which already use SPL FIT loading?
>>>>>
>>>>> ?And for the Pine64 part:
>>>>> ?5) Is extending the usable SPL size like in patch 5-7 acceptable?
>>>>>
>>>>> ?I have a more generic solution for the .dtb selection in mind: Based on
>>>>> ?some patch from Siarhei we store the .dtb filename in the SPL header and
>>>>> ?select the .dtb from the FIT image by simply matching the name. This would
>>>>> ?allow _one_ build supporting multiple boards. The actual board name would
>>>>> ?need to written into the SPL header or could be copied from there when
>>>>> ?updating the image.
>>>>> ?I can provide the patches once we agreed upon this series.
>>>>>
>>>>> ?Please let me know what you think!
>>>>>
>>>>> ?Cheers,
>>>>> ?Andre.
>>>>>
>>>>> ?Andre Przywara (11):
>>>>> ???SPL: FIT: refactor FDT loading
>>>>> ???SPL: FIT: rework U-Boot image loading
>>>>> ???SPL: FIT: factor out spl_load_fit_image()
>>>>> ???SPL: FIT: allow loading multiple images
>>>>> ???tools: mksunxiboot: allow larger SPL binaries
>>>>> ???sunxi: A64: SPL: allow large SPL binary
>>>>> ???sunxi: A64: move SPL stack to end of SRAM A2
>>>>> ???sunxi: SPL: add FIT config selector for Pine64 boards
>>>>> ???sunxi: Pine64: defconfig: enable SPL FIT support
>>>>> ???sunxi: Pine64: add FIT image source
>>>>> ???SPL: SPI: sunxi: add SPL FIT image support
>>>>>
>>>>> ??board/sunxi/board.c | 13 +++
>>>>> ??board/sunxi/pine64_atf.its | 54 +++++++++
>>>>> ??common/spl/spl_fit.c | 241 +++++++++++++++++++++++-----------------
>>>>> ??configs/pine64_plus_defconfig | 5 +
>>>>> ??drivers/mtd/spi/sunxi_spi_spl.c | 39 +++++--
>>>>> ??include/configs/sunxi-common.h | 6 +-
>>>>> ??scripts/Makefile.spl | 7 +-
>>>>> ??tools/mksunxiboot.c | 51 ++++++---
>>>>> ??8 files changed, 290 insertions(+), 126 deletions(-)
>>>>> ??create mode 100644 board/sunxi/pine64_atf.its
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[André Przywara]

Hi,

On 20/01/17 17:35, Andrew F. Davis wrote:
> On 01/20/2017 11:17 AM, Andre Przywara wrote:
>> Hi Andrew,
>>
>> thanks for the comments.
>>
>> On 20/01/17 17:02, Andrew F. Davis wrote:
>>> On 01/19/2017 07:53 PM, Andre Przywara wrote:
>>>> Currently the FIT format is not used to its full potential in the SPL:
>>>> It only loads the first image from the /images node and appends the
>>>> proper FDT.
>>>> Some boards and platforms would benefit from loading more images before
>>>> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
>>>> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>>>>
>>>> This series tries to solve this in a board agnostic and generic way:
>>>> We extend the SPL FIT loading scheme to allow loading multiple images.
>>>> So apart from loading the image which is referenced by the "firmware"
>>>> property in the respective configuration node and placing the DTB right
>>>> behind it, we iterate over all strings in the "loadable" property.
>>>> Each image referenced there will be loaded to its specified load address.
>>>> The entry point U-Boot eventually branches to will be taken from the
>>>> first image to explicitly provide the "entry" property, or, if none
>>>> of them does so, from the load address of the "firmware" image.
>>>> This keeps the scheme compatible with the FIT images our Makefile creates
>>>> automatically at the moment.
>>>>
>>>> Apart from the already mentioned ATF scenario this opens up more usage
>>>> scenarios, of which the commit message of patch 04/11 lists some.
>>>>
>>>
>>> I have been thinking about a similar problem we are facing regarding
>>> OP-TEE loading when doing SPL-only boots. I think extending SPL FIT
>>> loader is a good approach, I've just been concerned about SPL bloat. On
>>> our High Security enabled AM335x SoC we only have 41KB of SRAM to load
>>> SPL (last I checked we only have 100 bytes of overhead left), and we
>>> need FIT support as we use it for image authentication (it's what we use
>>> the board_fit_image_post_process() hook for), so any changes to SPL FIT
>>> need to be carefully made.
>>
>> Understood. Have you actually included FIT support as it exists already
>> in your build? My observation is that even without these changes
>> proposed here SPL FIT adds quite a bit to the SPL size (hence my patches
>> to extend SPL size for AArch64 sunxi). This is mostly due to some libfdt
>> bits being included, so I guess there is not much you can do about it. I
>> will later check how much the code size changes with my patches.
>> And btw.: Allwinner has either 24KB or 32KB of SRAM, so the situation is
>> even more dire here.

So that's the situation for the Orangepi Zero (which is ARMv7), just
quick test with GCC 5.2.0 (I believe newer version can generate smaller
code):
opi-zero-defconfig: SPL-SPI, no FIT at all
   text    data     bss     dec     hex filename
  17536     432     232   18200    4718 spl/u-boot-spl
opi-zero-defconfig: SPL-SPI, current FIT SPL
   text    data     bss     dec     hex filename
  20100     432     232   20764    511c spl/u-boot-spl
opi-zero-defconfig: SPL-SPI, extended FIT SPL
   text    data     bss     dec     hex filename
  20313     432     232   20977    51f1 spl/u-boot-spl

So this costs us 213 Bytes.
Let me send you this ctype() hack I talked about, that saves quite
something for me:
opi-zero-defconfig: SPL-SPI, extended FIT SPL, tiny_ctype
   text    data     bss     dec     hex filename
  20069     432     232   20733    50fd spl/u-boot-spl

Cheers,
Andre.

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Simon Glass]

Hi Andre,

On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote:
> Currently the FIT format is not used to its full potential in the SPL:
> It only loads the first image from the /images node and appends the
> proper FDT.
> Some boards and platforms would benefit from loading more images before
> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>
> This series tries to solve this in a board agnostic and generic way:
> We extend the SPL FIT loading scheme to allow loading multiple images.
> So apart from loading the image which is referenced by the "firmware"
> property in the respective configuration node and placing the DTB right
> behind it, we iterate over all strings in the "loadable" property.
> Each image referenced there will be loaded to its specified load address.
> The entry point U-Boot eventually branches to will be taken from the
> first image to explicitly provide the "entry" property, or, if none
> of them does so, from the load address of the "firmware" image.
> This keeps the scheme compatible with the FIT images our Makefile creates
> automatically at the moment.
>
> Apart from the already mentioned ATF scenario this opens up more usage
> scenarios, of which the commit message of patch 04/11 lists some.
>
> The first three patches rework the SPL FIT support to be more flexible
> and to allow easier usage in the fourth patch, which introduces the
> multiple-image loading facility.
> The remaining patches enable that support for the Pine64 board to make
> its SPL support finally useful and to demonstrate usage of this scheme:
> patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
> compilation of the SPL with FIT support enabled. Patch 8 implements the
> board selector routine, which selects either the Pine64 or Pine64+ DTB
> depending on the detected DRAM size. Patch 9 enables SPL FIT support in
> the Pine64 defconfig.
> To demonstrate the usage, patch 10 provides a FIT source file, which
> loads and executes ATF before the U-Boot proper. Users are expected to
> compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
> then write the resulting file behind the SPL on an SD card (or any other
> U-Boot supported boot media, for that matter).
> Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
> which allows to load ATF on boards with SPI flash as well.
>
> Questions:
> 1) Is this scheme the right one (usage of "firmware" and "loadables",
>    determination of entry point)? Shall we make use of the "setup"
>    property?

Seems reasonable to me.

> 2) Shall we extend mkimage to allow supplying "loadable" files on the
>    command line, which would allow to build the .itb file automatically?

Yes.

> 3) Is providing the .its source file for a (family of) boards the right
>    way?

Where needed (i.e. the mkimage command line is not trivial)

> 4) Does this break any boards which already use SPL FIT loading?

Probably not, but I'm sure it would be spotted if so.

>
> And for the Pine64 part:
> 5) Is extending the usable SPL size like in patch 5-7 acceptable?
>
> I have a more generic solution for the .dtb selection in mind: Based on
> some patch from Siarhei we store the .dtb filename in the SPL header and
> select the .dtb from the FIT image by simply matching the name. This would
> allow _one_ build supporting multiple boards. The actual board name would
> need to written into the SPL header or could be copied from there when
> updating the image.
> I can provide the patches once we agreed upon this series.
>
> Please let me know what you think!

I think it is a nice piece of work. You might want to make the new
feature optional if it saves code space.

FIT is preferable to a raw binary image. There is also binman which
can create binary images with things placed as you wish, but it's best
to use FIT where you can.

A few more things:

- Can you make sure that your new node structure is clearly documented
(with an example) in doc/uImage.FIT/ ?
- Can you add a pytest for sandbox_spl, which verifies that everything
is loaded correctly? As an alternative I suppose you could adjust
test-fit.py (which should move to pytest)

Regards,
Simon

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[André Przywara]

On 27/01/17 21:29, Simon Glass wrote:

Hi Simon,

> On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote:
>> Currently the FIT format is not used to its full potential in the SPL:
>> It only loads the first image from the /images node and appends the
>> proper FDT.
>> Some boards and platforms would benefit from loading more images before
>> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
>> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>>
>> This series tries to solve this in a board agnostic and generic way:
>> We extend the SPL FIT loading scheme to allow loading multiple images.
>> So apart from loading the image which is referenced by the "firmware"
>> property in the respective configuration node and placing the DTB right
>> behind it, we iterate over all strings in the "loadable" property.
>> Each image referenced there will be loaded to its specified load address.
>> The entry point U-Boot eventually branches to will be taken from the
>> first image to explicitly provide the "entry" property, or, if none
>> of them does so, from the load address of the "firmware" image.
>> This keeps the scheme compatible with the FIT images our Makefile creates
>> automatically at the moment.
>>
>> Apart from the already mentioned ATF scenario this opens up more usage
>> scenarios, of which the commit message of patch 04/11 lists some.
>>
>> The first three patches rework the SPL FIT support to be more flexible
>> and to allow easier usage in the fourth patch, which introduces the
>> multiple-image loading facility.
>> The remaining patches enable that support for the Pine64 board to make
>> its SPL support finally useful and to demonstrate usage of this scheme:
>> patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
>> compilation of the SPL with FIT support enabled. Patch 8 implements the
>> board selector routine, which selects either the Pine64 or Pine64+ DTB
>> depending on the detected DRAM size. Patch 9 enables SPL FIT support in
>> the Pine64 defconfig.
>> To demonstrate the usage, patch 10 provides a FIT source file, which
>> loads and executes ATF before the U-Boot proper. Users are expected to
>> compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
>> then write the resulting file behind the SPL on an SD card (or any other
>> U-Boot supported boot media, for that matter).
>> Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
>> which allows to load ATF on boards with SPI flash as well.
>>
>> Questions:
>> 1) Is this scheme the right one (usage of "firmware" and "loadables",
>>    determination of entry point)? Shall we make use of the "setup"
>>    property?
> 
> Seems reasonable to me.
> 
>> 2) Shall we extend mkimage to allow supplying "loadable" files on the
>>    command line, which would allow to build the .itb file automatically?
> 
> Yes.

I was thinking about this a bit more, as Andrew pointed out before it
may become hairy to add tons of options to mkimage.
I came up with a simple shell script, mostly using here documents
(cat << _EOF) to generate the .its file on the fly, adding all DTs given
on the command line. It's pretty easy, yet readable and adaptable. So
each platform could provide one, if needed, and could hard code things
like ATF in here.

>> 3) Is providing the .its source file for a (family of) boards the right
>>    way?
> 
> Where needed (i.e. the mkimage command line is not trivial)
> 
>> 4) Does this break any boards which already use SPL FIT loading?
> 
> Probably not, but I'm sure it would be spotted if so.
> 
>>
>> And for the Pine64 part:
>> 5) Is extending the usable SPL size like in patch 5-7 acceptable?
>>
>> I have a more generic solution for the .dtb selection in mind: Based on
>> some patch from Siarhei we store the .dtb filename in the SPL header and
>> select the .dtb from the FIT image by simply matching the name. This would
>> allow _one_ build supporting multiple boards. The actual board name would
>> need to written into the SPL header or could be copied from there when
>> updating the image.
>> I can provide the patches once we agreed upon this series.
>>
>> Please let me know what you think!
> 
> I think it is a nice piece of work. You might want to make the new
> feature optional if it saves code space.
> 
> FIT is preferable to a raw binary image. There is also binman which
> can create binary images with things placed as you wish, but it's best
> to use FIT where you can.

Yeah, in this case it is not so much about placing, but more about
flexibility, so FIT is really nice here.
I am also looking at a firmware update tool atm, having FIT would allow
to update single components only - for instance just the .dtb.

> A few more things:
> 
> - Can you make sure that your new node structure is clearly documented
> (with an example) in doc/uImage.FIT/ ?

Yes, sure.

> - Can you add a pytest for sandbox_spl, which verifies that everything
> is loaded correctly? As an alternative I suppose you could adjust
> test-fit.py (which should move to pytest)

OK, will look into this.

Thanks for the comments!

Cheers,
Andre.

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Simon Glass]

Hi Andre,

On 27 January 2017 at 17:47, Andr? Przywara <andre.przywara@arm.com> wrote:
> On 27/01/17 21:29, Simon Glass wrote:
>
> Hi Simon,
>
>> On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote:
>>> Currently the FIT format is not used to its full potential in the SPL:
>>> It only loads the first image from the /images node and appends the
>>> proper FDT.
>>> Some boards and platforms would benefit from loading more images before
>>> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
>>> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>>>
>>> This series tries to solve this in a board agnostic and generic way:
>>> We extend the SPL FIT loading scheme to allow loading multiple images.
>>> So apart from loading the image which is referenced by the "firmware"
>>> property in the respective configuration node and placing the DTB right
>>> behind it, we iterate over all strings in the "loadable" property.
>>> Each image referenced there will be loaded to its specified load address.
>>> The entry point U-Boot eventually branches to will be taken from the
>>> first image to explicitly provide the "entry" property, or, if none
>>> of them does so, from the load address of the "firmware" image.
>>> This keeps the scheme compatible with the FIT images our Makefile creates
>>> automatically at the moment.
>>>
>>> Apart from the already mentioned ATF scenario this opens up more usage
>>> scenarios, of which the commit message of patch 04/11 lists some.
>>>
>>> The first three patches rework the SPL FIT support to be more flexible
>>> and to allow easier usage in the fourth patch, which introduces the
>>> multiple-image loading facility.
>>> The remaining patches enable that support for the Pine64 board to make
>>> its SPL support finally useful and to demonstrate usage of this scheme:
>>> patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
>>> compilation of the SPL with FIT support enabled. Patch 8 implements the
>>> board selector routine, which selects either the Pine64 or Pine64+ DTB
>>> depending on the detected DRAM size. Patch 9 enables SPL FIT support in
>>> the Pine64 defconfig.
>>> To demonstrate the usage, patch 10 provides a FIT source file, which
>>> loads and executes ATF before the U-Boot proper. Users are expected to
>>> compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
>>> then write the resulting file behind the SPL on an SD card (or any other
>>> U-Boot supported boot media, for that matter).
>>> Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
>>> which allows to load ATF on boards with SPI flash as well.
>>>
>>> Questions:
>>> 1) Is this scheme the right one (usage of "firmware" and "loadables",
>>>    determination of entry point)? Shall we make use of the "setup"
>>>    property?
>>
>> Seems reasonable to me.
>>
>>> 2) Shall we extend mkimage to allow supplying "loadable" files on the
>>>    command line, which would allow to build the .itb file automatically?
>>
>> Yes.
>
> I was thinking about this a bit more, as Andrew pointed out before it
> may become hairy to add tons of options to mkimage.
> I came up with a simple shell script, mostly using here documents
> (cat << _EOF) to generate the .its file on the fly, adding all DTs given
> on the command line. It's pretty easy, yet readable and adaptable. So
> each platform could provide one, if needed, and could hard code things
> like ATF in here.

That sounds reasonable. But I do think it is valuable to support the
basic case without needing a script, so long as you can do it with
only a few mkimage options?

[..]

Regards,
Simon

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Andre Przywara]

Hi Simon,

On 06/02/17 15:33, Simon Glass wrote:
> Hi Andre,
> 
> On 27 January 2017 at 17:47, Andr? Przywara <andre.przywara@arm.com> wrote:
>> On 27/01/17 21:29, Simon Glass wrote:
>>
>> Hi Simon,
>>
>>> On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote:
>>>> Currently the FIT format is not used to its full potential in the SPL:
>>>> It only loads the first image from the /images node and appends the
>>>> proper FDT.
>>>> Some boards and platforms would benefit from loading more images before
>>>> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
>>>> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>>>>
>>>> This series tries to solve this in a board agnostic and generic way:
>>>> We extend the SPL FIT loading scheme to allow loading multiple images.
>>>> So apart from loading the image which is referenced by the "firmware"
>>>> property in the respective configuration node and placing the DTB right
>>>> behind it, we iterate over all strings in the "loadable" property.
>>>> Each image referenced there will be loaded to its specified load address.
>>>> The entry point U-Boot eventually branches to will be taken from the
>>>> first image to explicitly provide the "entry" property, or, if none
>>>> of them does so, from the load address of the "firmware" image.
>>>> This keeps the scheme compatible with the FIT images our Makefile creates
>>>> automatically at the moment.
>>>>
>>>> Apart from the already mentioned ATF scenario this opens up more usage
>>>> scenarios, of which the commit message of patch 04/11 lists some.
>>>>
>>>> The first three patches rework the SPL FIT support to be more flexible
>>>> and to allow easier usage in the fourth patch, which introduces the
>>>> multiple-image loading facility.
>>>> The remaining patches enable that support for the Pine64 board to make
>>>> its SPL support finally useful and to demonstrate usage of this scheme:
>>>> patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
>>>> compilation of the SPL with FIT support enabled. Patch 8 implements the
>>>> board selector routine, which selects either the Pine64 or Pine64+ DTB
>>>> depending on the detected DRAM size. Patch 9 enables SPL FIT support in
>>>> the Pine64 defconfig.
>>>> To demonstrate the usage, patch 10 provides a FIT source file, which
>>>> loads and executes ATF before the U-Boot proper. Users are expected to
>>>> compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
>>>> then write the resulting file behind the SPL on an SD card (or any other
>>>> U-Boot supported boot media, for that matter).
>>>> Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
>>>> which allows to load ATF on boards with SPI flash as well.
>>>>
>>>> Questions:
>>>> 1) Is this scheme the right one (usage of "firmware" and "loadables",
>>>>    determination of entry point)? Shall we make use of the "setup"
>>>>    property?
>>>
>>> Seems reasonable to me.
>>>
>>>> 2) Shall we extend mkimage to allow supplying "loadable" files on the
>>>>    command line, which would allow to build the .itb file automatically?
>>>
>>> Yes.
>>
>> I was thinking about this a bit more, as Andrew pointed out before it
>> may become hairy to add tons of options to mkimage.
>> I came up with a simple shell script, mostly using here documents
>> (cat << _EOF) to generate the .its file on the fly, adding all DTs given
>> on the command line. It's pretty easy, yet readable and adaptable. So
>> each platform could provide one, if needed, and could hard code things
>> like ATF in here.
> 
> That sounds reasonable. But I do think it is valuable to support the
> basic case without needing a script, so long as you can do it with
> only a few mkimage options?

Well, the problem is that aside from the loadable name we would need to
communicate at least the load address, probably also the entry point. So
we could go with:
$ mkimage ... -l 0x123400:some.img -l 0x876500:another.img
to specify at least the load address, but that wouldn't cover entry
points or other parameters. I think at this point it becomes a bit messy.
My understanding is that using FDT as the base for a FIT image allows us
to flexibly describe this in a source file, so we should use that
features instead of stuffing more special cases into mkimage.
My current thinking is:
- there can be a CONFIG_FIT_SOURCE variable, which points to a board (or
platform) specific .its source
- there can be a CONFIG_FIT_GENERATOR symbol, which points to a
generator _script_
The Makefile checks for either of those and generates the image file
accordingly. In case it calls the generator script, it passes
CONFIG_OF_LIST (or other parameters). I am in the process of stuffing
this into the Makefile, which is not fun, really ;-)

So what do you think: Is it still worth to enhance mkimage?
I would prototype this CONFIG_FIT_ approach now and we can discuss this
in the series, then, I guess.

Cheers,
Andre.

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Andrew F. Davis]

On 02/06/2017 09:33 AM, Simon Glass wrote:
> Hi Andre,
> 
> On 27 January 2017 at 17:47, Andr? Przywara <andre.przywara@arm.com> wrote:
>> On 27/01/17 21:29, Simon Glass wrote:
>>
>> Hi Simon,
>>
>>> On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote:
>>>> Currently the FIT format is not used to its full potential in the SPL:
>>>> It only loads the first image from the /images node and appends the
>>>> proper FDT.
>>>> Some boards and platforms would benefit from loading more images before
>>>> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
>>>> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>>>>
>>>> This series tries to solve this in a board agnostic and generic way:
>>>> We extend the SPL FIT loading scheme to allow loading multiple images.
>>>> So apart from loading the image which is referenced by the "firmware"
>>>> property in the respective configuration node and placing the DTB right
>>>> behind it, we iterate over all strings in the "loadable" property.
>>>> Each image referenced there will be loaded to its specified load address.
>>>> The entry point U-Boot eventually branches to will be taken from the
>>>> first image to explicitly provide the "entry" property, or, if none
>>>> of them does so, from the load address of the "firmware" image.
>>>> This keeps the scheme compatible with the FIT images our Makefile creates
>>>> automatically at the moment.
>>>>
>>>> Apart from the already mentioned ATF scenario this opens up more usage
>>>> scenarios, of which the commit message of patch 04/11 lists some.
>>>>
>>>> The first three patches rework the SPL FIT support to be more flexible
>>>> and to allow easier usage in the fourth patch, which introduces the
>>>> multiple-image loading facility.
>>>> The remaining patches enable that support for the Pine64 board to make
>>>> its SPL support finally useful and to demonstrate usage of this scheme:
>>>> patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
>>>> compilation of the SPL with FIT support enabled. Patch 8 implements the
>>>> board selector routine, which selects either the Pine64 or Pine64+ DTB
>>>> depending on the detected DRAM size. Patch 9 enables SPL FIT support in
>>>> the Pine64 defconfig.
>>>> To demonstrate the usage, patch 10 provides a FIT source file, which
>>>> loads and executes ATF before the U-Boot proper. Users are expected to
>>>> compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
>>>> then write the resulting file behind the SPL on an SD card (or any other
>>>> U-Boot supported boot media, for that matter).
>>>> Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
>>>> which allows to load ATF on boards with SPI flash as well.
>>>>
>>>> Questions:
>>>> 1) Is this scheme the right one (usage of "firmware" and "loadables",
>>>>    determination of entry point)? Shall we make use of the "setup"
>>>>    property?
>>>
>>> Seems reasonable to me.
>>>
>>>> 2) Shall we extend mkimage to allow supplying "loadable" files on the
>>>>    command line, which would allow to build the .itb file automatically?
>>>
>>> Yes.
>>
>> I was thinking about this a bit more, as Andrew pointed out before it
>> may become hairy to add tons of options to mkimage.
>> I came up with a simple shell script, mostly using here documents
>> (cat << _EOF) to generate the .its file on the fly, adding all DTs given
>> on the command line. It's pretty easy, yet readable and adaptable. So
>> each platform could provide one, if needed, and could hard code things
>> like ATF in here.
> 
> That sounds reasonable. But I do think it is valuable to support the
> basic case without needing a script, so long as you can do it with
> only a few mkimage options?
> 

We already have a few mkimage option for auto-generating FIT for basic
cases (Executable and DTBs). I've seen internally confusion caused by
having mkimage accept dtb's on the command-line while we work to add
more complex FIT schemes. I think our time is best spent working on
simplifying generating custom .its files during build, and less on
patching mkimage to support increasingly complex build configurations.

How about we have template .its files for platforms and for simple build
cases, then simply define a way to fill in these using mkimage:

> / {
>         description = "U-Boot fitImage for PlatformX";
>         #address-cells = <1>;
> 
>         images {
>                 u-boot at 1 {
>                         description = "U-Boot";
>                         data = /incbin/("u-boot.bin");
>                         arch = "arm";
>                         load = <[u_boot_load]>;
>                 };
>                 [dtb_name] {
>                         description = "Flattened Device Tree blob";
>                         data = /incbin/("arch/arm/boot/dts/[dtb_name].dtb");
>                         type = "flat_dt";
>                         arch = "arm";
>                 };
>                 firmware at 1 {
>                         data = /incbin/("[firmware_name]");
>                         arch = "arm";
>                         compression = "none";
>                 };
>         };
> };

Then:

> $ mkimage --template "default.dtb" -x "u_boot_load=0x800000,dtb_name=dra7xx_evm.dtb,firmware_name=atf.bin"

Or better yet, use an existing template engine as a pre-processing step
in the Makefile to generate the needed .its files.

Andrew

> 
> Regards,
> Simon
> 

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Andre Przywara]

Hi Andrew,

On 06/02/17 16:17, Andrew F. Davis wrote:
> On 02/06/2017 09:33 AM, Simon Glass wrote:
>> Hi Andre,
>>
>> On 27 January 2017 at 17:47, Andr? Przywara <andre.przywara@arm.com> wrote:
>>> On 27/01/17 21:29, Simon Glass wrote:
>>>
>>> Hi Simon,
>>>
>>>> On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote:
>>>>> Currently the FIT format is not used to its full potential in the SPL:
>>>>> It only loads the first image from the /images node and appends the
>>>>> proper FDT.
>>>>> Some boards and platforms would benefit from loading more images before
>>>>> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
>>>>> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>>>>>
>>>>> This series tries to solve this in a board agnostic and generic way:
>>>>> We extend the SPL FIT loading scheme to allow loading multiple images.
>>>>> So apart from loading the image which is referenced by the "firmware"
>>>>> property in the respective configuration node and placing the DTB right
>>>>> behind it, we iterate over all strings in the "loadable" property.
>>>>> Each image referenced there will be loaded to its specified load address.
>>>>> The entry point U-Boot eventually branches to will be taken from the
>>>>> first image to explicitly provide the "entry" property, or, if none
>>>>> of them does so, from the load address of the "firmware" image.
>>>>> This keeps the scheme compatible with the FIT images our Makefile creates
>>>>> automatically at the moment.
>>>>>
>>>>> Apart from the already mentioned ATF scenario this opens up more usage
>>>>> scenarios, of which the commit message of patch 04/11 lists some.
>>>>>
>>>>> The first three patches rework the SPL FIT support to be more flexible
>>>>> and to allow easier usage in the fourth patch, which introduces the
>>>>> multiple-image loading facility.
>>>>> The remaining patches enable that support for the Pine64 board to make
>>>>> its SPL support finally useful and to demonstrate usage of this scheme:
>>>>> patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
>>>>> compilation of the SPL with FIT support enabled. Patch 8 implements the
>>>>> board selector routine, which selects either the Pine64 or Pine64+ DTB
>>>>> depending on the detected DRAM size. Patch 9 enables SPL FIT support in
>>>>> the Pine64 defconfig.
>>>>> To demonstrate the usage, patch 10 provides a FIT source file, which
>>>>> loads and executes ATF before the U-Boot proper. Users are expected to
>>>>> compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
>>>>> then write the resulting file behind the SPL on an SD card (or any other
>>>>> U-Boot supported boot media, for that matter).
>>>>> Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
>>>>> which allows to load ATF on boards with SPI flash as well.
>>>>>
>>>>> Questions:
>>>>> 1) Is this scheme the right one (usage of "firmware" and "loadables",
>>>>>    determination of entry point)? Shall we make use of the "setup"
>>>>>    property?
>>>>
>>>> Seems reasonable to me.
>>>>
>>>>> 2) Shall we extend mkimage to allow supplying "loadable" files on the
>>>>>    command line, which would allow to build the .itb file automatically?
>>>>
>>>> Yes.
>>>
>>> I was thinking about this a bit more, as Andrew pointed out before it
>>> may become hairy to add tons of options to mkimage.
>>> I came up with a simple shell script, mostly using here documents
>>> (cat << _EOF) to generate the .its file on the fly, adding all DTs given
>>> on the command line. It's pretty easy, yet readable and adaptable. So
>>> each platform could provide one, if needed, and could hard code things
>>> like ATF in here.
>>
>> That sounds reasonable. But I do think it is valuable to support the
>> basic case without needing a script, so long as you can do it with
>> only a few mkimage options?
>>
> 
> We already have a few mkimage option for auto-generating FIT for basic
> cases (Executable and DTBs). I've seen internally confusion caused by
> having mkimage accept dtb's on the command-line while we work to add
> more complex FIT schemes. I think our time is best spent working on
> simplifying generating custom .its files during build, and less on
> patching mkimage to support increasingly complex build configurations.
> 
> How about we have template .its files for platforms and for simple build
> cases, then simply define a way to fill in these using mkimage:
> 
>> / {
>>         description = "U-Boot fitImage for PlatformX";
>>         #address-cells = <1>;
>>
>>         images {
>>                 u-boot at 1 {
>>                         description = "U-Boot";
>>                         data = /incbin/("u-boot.bin");
>>                         arch = "arm";
>>                         load = <[u_boot_load]>;
>>                 };
>>                 [dtb_name] {
>>                         description = "Flattened Device Tree blob";
>>                         data = /incbin/("arch/arm/boot/dts/[dtb_name].dtb");
>>                         type = "flat_dt";
>>                         arch = "arm";
>>                 };
>>                 firmware at 1 {
>>                         data = /incbin/("[firmware_name]");
>>                         arch = "arm";
>>                         compression = "none";
>>                 };
>>         };
>> };
> 
> Then:
> 
>> $ mkimage --template "default.dtb" -x "u_boot_load=0x800000,dtb_name=dra7xx_evm.dtb,firmware_name=atf.bin"
> 
> Or better yet, use an existing template engine as a pre-processing step
> in the Makefile to generate the needed .its files.

Short of using some fancy templating, would that script cover that?

==================================================
#!/bin/sh
#
# script to generate FIT image source for 64-bit sunxi boards with
# ARM Trusted Firmware and multiple device trees (given on the command #
line)
#
# usage: $0 <dt_name> [<dt_name> [<dt_name] ...]

cat << __HEADER_EOF
/dts-v1/;

/ {
        description = "Configuration to load ATF before U-Boot";
        #address-cells = <1>;

        images {
                uboot at 1 {
                        description = "U-Boot (64-bit)";
                        data = /incbin/("u-boot-nodtb.bin");
                        type = "standalone";
                        arch = "arm64";
                        compression = "none";
                        load = <0x4a000000>;
                };
                atf at 1 {
                        description = "ARM Trusted Firmware";
                        data = /incbin/("bl31.bin");
                        type = "firmware";
                        arch = "arm64";
                        compression = "none";
                        load = <0x44000>;
                        entry = <0x44000>;
                };
__HEADER_EOF

cnt=1
for dtname in $*
do
        cat << __FDT_IMAGE_EOF
                fdt@$cnt {
                        description = "$dtname";
                        data = /incbin/("arch/arm/dts/$dtname.dtb");
                        type = "flat_dt";
                        compression = "none";
                };
__FDT_IMAGE_EOF
        cnt=$((cnt+1))
done

cat << __CONF_HEADER_EOF
        };
        configurations {
                default = "config at 1";

__CONF_HEADER_EOF

cnt=1
for dtname in $*
do
        cat << __CONF_SECTION_EOF
                config@$cnt {
                        description = "$dtname";
                        firmware = "uboot at 1";
                        loadables = "atf@1";
                        fdt = "fdt@$cnt";
                };
__CONF_SECTION_EOF
        cnt=$((cnt+1))
done

cat << __ITS_EOF
        };
};
__ITS_EOF
==================================================

This would be passed $CONFIG_OF_LIST.
The resulting .its file can the be transformed by a generic:
mkimage -f u-boot.its -E u-boot.img

This script would be platform specific and referenced by boards in need
for it.

Andrew, does that cover the use cases you were thinking of?

Cheers,
Andre.

[U-Boot] [RFC PATCH 00/11] extend FIT loading support (plus Pine64/ATF support)

[Andrew F. Davis]

On 02/06/2017 10:32 AM, Andre Przywara wrote:
> Hi Andrew,
> 
> On 06/02/17 16:17, Andrew F. Davis wrote:
>> On 02/06/2017 09:33 AM, Simon Glass wrote:
>>> Hi Andre,
>>>
>>> On 27 January 2017 at 17:47, Andr? Przywara <andre.przywara@arm.com> wrote:
>>>> On 27/01/17 21:29, Simon Glass wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>>> On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote:
>>>>>> Currently the FIT format is not used to its full potential in the SPL:
>>>>>> It only loads the first image from the /images node and appends the
>>>>>> proper FDT.
>>>>>> Some boards and platforms would benefit from loading more images before
>>>>>> starting U-Boot proper, notably Allwinner A64 and ARMv8 Rockchip boards,
>>>>>> which use an ARM Trusted Firmware (ATF) image to be executed before U-Boot.
>>>>>>
>>>>>> This series tries to solve this in a board agnostic and generic way:
>>>>>> We extend the SPL FIT loading scheme to allow loading multiple images.
>>>>>> So apart from loading the image which is referenced by the "firmware"
>>>>>> property in the respective configuration node and placing the DTB right
>>>>>> behind it, we iterate over all strings in the "loadable" property.
>>>>>> Each image referenced there will be loaded to its specified load address.
>>>>>> The entry point U-Boot eventually branches to will be taken from the
>>>>>> first image to explicitly provide the "entry" property, or, if none
>>>>>> of them does so, from the load address of the "firmware" image.
>>>>>> This keeps the scheme compatible with the FIT images our Makefile creates
>>>>>> automatically at the moment.
>>>>>>
>>>>>> Apart from the already mentioned ATF scenario this opens up more usage
>>>>>> scenarios, of which the commit message of patch 04/11 lists some.
>>>>>>
>>>>>> The first three patches rework the SPL FIT support to be more flexible
>>>>>> and to allow easier usage in the fourth patch, which introduces the
>>>>>> multiple-image loading facility.
>>>>>> The remaining patches enable that support for the Pine64 board to make
>>>>>> its SPL support finally useful and to demonstrate usage of this scheme:
>>>>>> patches 5-7 extend the usable SPL size by about 4 KB to allow AArch64
>>>>>> compilation of the SPL with FIT support enabled. Patch 8 implements the
>>>>>> board selector routine, which selects either the Pine64 or Pine64+ DTB
>>>>>> depending on the detected DRAM size. Patch 9 enables SPL FIT support in
>>>>>> the Pine64 defconfig.
>>>>>> To demonstrate the usage, patch 10 provides a FIT source file, which
>>>>>> loads and executes ATF before the U-Boot proper. Users are expected to
>>>>>> compile this with "mkimage -f boards/sunxi/pine64_atf.its -E pine64.itb",
>>>>>> then write the resulting file behind the SPL on an SD card (or any other
>>>>>> U-Boot supported boot media, for that matter).
>>>>>> Patch 11 then adds FIT support to the sunxi SPL SPI loading routine,
>>>>>> which allows to load ATF on boards with SPI flash as well.
>>>>>>
>>>>>> Questions:
>>>>>> 1) Is this scheme the right one (usage of "firmware" and "loadables",
>>>>>>    determination of entry point)? Shall we make use of the "setup"
>>>>>>    property?
>>>>>
>>>>> Seems reasonable to me.
>>>>>
>>>>>> 2) Shall we extend mkimage to allow supplying "loadable" files on the
>>>>>>    command line, which would allow to build the .itb file automatically?
>>>>>
>>>>> Yes.
>>>>
>>>> I was thinking about this a bit more, as Andrew pointed out before it
>>>> may become hairy to add tons of options to mkimage.
>>>> I came up with a simple shell script, mostly using here documents
>>>> (cat << _EOF) to generate the .its file on the fly, adding all DTs given
>>>> on the command line. It's pretty easy, yet readable and adaptable. So
>>>> each platform could provide one, if needed, and could hard code things
>>>> like ATF in here.
>>>
>>> That sounds reasonable. But I do think it is valuable to support the
>>> basic case without needing a script, so long as you can do it with
>>> only a few mkimage options?
>>>
>>
>> We already have a few mkimage option for auto-generating FIT for basic
>> cases (Executable and DTBs). I've seen internally confusion caused by
>> having mkimage accept dtb's on the command-line while we work to add
>> more complex FIT schemes. I think our time is best spent working on
>> simplifying generating custom .its files during build, and less on
>> patching mkimage to support increasingly complex build configurations.
>>
>> How about we have template .its files for platforms and for simple build
>> cases, then simply define a way to fill in these using mkimage:
>>
>>> / {
>>>         description = "U-Boot fitImage for PlatformX";
>>>         #address-cells = <1>;
>>>
>>>         images {
>>>                 u-boot at 1 {
>>>                         description = "U-Boot";
>>>                         data = /incbin/("u-boot.bin");
>>>                         arch = "arm";
>>>                         load = <[u_boot_load]>;
>>>                 };
>>>                 [dtb_name] {
>>>                         description = "Flattened Device Tree blob";
>>>                         data = /incbin/("arch/arm/boot/dts/[dtb_name].dtb");
>>>                         type = "flat_dt";
>>>                         arch = "arm";
>>>                 };
>>>                 firmware at 1 {
>>>                         data = /incbin/("[firmware_name]");
>>>                         arch = "arm";
>>>                         compression = "none";
>>>                 };
>>>         };
>>> };
>>
>> Then:
>>
>>> $ mkimage --template "default.dtb" -x "u_boot_load=0x800000,dtb_name=dra7xx_evm.dtb,firmware_name=atf.bin"
>>
>> Or better yet, use an existing template engine as a pre-processing step
>> in the Makefile to generate the needed .its files.
> 
> Short of using some fancy templating, would that script cover that?
> 
> ==================================================
> #!/bin/sh
> #
> # script to generate FIT image source for 64-bit sunxi boards with
> # ARM Trusted Firmware and multiple device trees (given on the command #
> line)
> #
> # usage: $0 <dt_name> [<dt_name> [<dt_name] ...]
> 
> cat << __HEADER_EOF
> /dts-v1/;
> 
> / {
>         description = "Configuration to load ATF before U-Boot";
>         #address-cells = <1>;
> 
>         images {
>                 uboot at 1 {
>                         description = "U-Boot (64-bit)";
>                         data = /incbin/("u-boot-nodtb.bin");
>                         type = "standalone";
>                         arch = "arm64";
>                         compression = "none";
>                         load = <0x4a000000>;
>                 };
>                 atf at 1 {
>                         description = "ARM Trusted Firmware";
>                         data = /incbin/("bl31.bin");
>                         type = "firmware";
>                         arch = "arm64";
>                         compression = "none";
>                         load = <0x44000>;
>                         entry = <0x44000>;
>                 };
> __HEADER_EOF
> 
> cnt=1
> for dtname in $*
> do
>         cat << __FDT_IMAGE_EOF
>                 fdt@$cnt {
>                         description = "$dtname";
>                         data = /incbin/("arch/arm/dts/$dtname.dtb");
>                         type = "flat_dt";
>                         compression = "none";
>                 };
> __FDT_IMAGE_EOF
>         cnt=$((cnt+1))
> done
> 
> cat << __CONF_HEADER_EOF
>         };
>         configurations {
>                 default = "config at 1";
> 
> __CONF_HEADER_EOF
> 
> cnt=1
> for dtname in $*
> do
>         cat << __CONF_SECTION_EOF
>                 config@$cnt {
>                         description = "$dtname";
>                         firmware = "uboot at 1";
>                         loadables = "atf at 1";
>                         fdt = "fdt@$cnt";
>                 };
> __CONF_SECTION_EOF
>         cnt=$((cnt+1))
> done
> 
> cat << __ITS_EOF
>         };
> };
> __ITS_EOF
> ==================================================
> 
> This would be passed $CONFIG_OF_LIST.
> The resulting .its file can the be transformed by a generic:
> mkimage -f u-boot.its -E u-boot.img
> 
> This script would be platform specific and referenced by boards in need
> for it.
> 
> Andrew, does that cover the use cases you were thinking of?
> 

I just saw your other reply after I had sent mine here, I like the idea
of having a CONFIG_FIT_GENERATOR symbol, or some other way to point to
platform specific generators, then you can use your style, and I can
make a more complex fancy templating one if I needed.

To answer your question, yes, for now it looks like that script could be
make to handle our current use-cases.

Thanks for posting that by the way, +Franklin who was looking for
something similar.

Andrew

> Cheers,
> Andre.
> 

[U-Boot] [linux-sunxi] Re: [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[Icenowy Zheng]

2017?1?24? 01:29? Maxime Ripard <maxime.ripard@free-electrons.com>???
>
> On Sat, Jan 21, 2017 at 03:15:27PM +0000, Andr? Przywara wrote: 
> > >>> On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:? 
> > >>>> For a board or platform to support FIT loading in the SPL, it has to 
> > >>>> provide a board_fit_config_name_match() routine, which helps to select 
> > >>>> one of possibly multiple DTBs contained in a FIT image. 
> > >>>> Provide a simple function to cover the two different Pine64 models, 
> > >>>> which can be easily told apart by looking at the amount of installed 
> > >>>> RAM. 
> > >>>> 
> > >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> 
> > >>>> --- 
> > >>>>? board/sunxi/board.c | 13 +++++++++++++ 
> > >>>>? 1 file changed, 13 insertions(+) 
> > >>>> 
> > >>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c 
> > >>>> index 5365638..bbbb826 100644 
> > >>>> --- a/board/sunxi/board.c 
> > >>>> +++ b/board/sunxi/board.c 
> > >>>> @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd) 
> > >>>>? #endif 
> > >>>>? return 0; 
> > >>>>? } 
> > >>>> + 
> > >>>> +#ifdef CONFIG_SPL_LOAD_FIT 
> > >>>> +int board_fit_config_name_match(const char *name) 
> > >>>> +{ 
> > >>>> +#ifdef CONFIG_MACH_SUN50I 
> > >>>> + if ((gd->ram_size > 512 * 1024 * 1024)) 
> > >>>> + return !strcmp(name, "sun50i-a64-pine64-plus"); 
> > >>>> + else 
> > >>>> + return !strcmp(name, "sun50i-a64-pine64"); 
> > >>>> +#endif 
> > >>>> + return -1; 
> > >>>> +}? 
> > >>> 
> > >>> That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT 
> > >>> enabled will be considered a pine64 board?? 
> > >> 
> > >> Yes, at least for now, because that's the only A64 board we officially 
> > >> support so far. 
> > >> And yes again, it is fishy. It is more a demo or a placeholder for now, 
> > >> because you _need_ an implementation of board_fit_config_name_match() 
> > >> once you enable CONFIG_SPL_LOAD_FIT. 
> > >> 
> > >> One solution would be to have only one DTB supported per build, so 
> > >> board_fit_config_name_match() always returns 0. 
> > >> 
> > >> Another (better) solution would be to store the board name in the SPL 
> > >> header, which could be done later when flashing the image. Then this 
> > >> function would just return strcmp(name, spl_header_name) or 0 if no name 
> > >> is found for whatever reason. 
> > > 
> > > Yes, this is a good solution in the case if we have some non-removable 
> > > storage on the board (SPI NOR flash, NAND, EEPROM or something else). 
> > > We can discuss it separately. 
> > 
> > I totally agree. I rebased your patch to latest mainline already and 
> > will send out something later. 
> > 
> > > But the current generation of Pine64 boards don't have any 
> > > non-removable storage yet. My understanding is that you tried to 
> > > provide the DTB selection, which is based on circumstantial evidences, 
> > > such as the RAM size. IMHO this is also a valid selection method, 
> > > because it can reduce the number of required OS image variants. 
> > 
> > Yes, the point is that for U-Boot's purposes the only difference between 
> > the Pine64 and Pine64+ (apart from DRAM size, which is autodetected) is 
> > using MII vs. RGMII for the Ethernet PHY. The sun8i_emac driver gets 
> > this information from the DT only, so there is no point at all for 
> > different defconfigs. And the DRAM size is a safe indicator for that 
> > difference, at least if we confine our view to Pine64 boards. 
> > 
> > Now how other boards fit in here is a separate discussion IMO, so let's 
> > solve one problem after the other. 
> > I just thought that we could use: 
> > 
> > #ifdef CONFIG_SPL_LOAD_FIT 
> > int board_fit_config_name_match(const char *name) 
> > { 
> > return strcmp(name, CONFIG_DEFAULT_DEVICE_TREE); 
> > } 
> > #endif 
>
> I guess an easy way around this would be to add a Kconfig option for 
> the pine64, like I tried to do for the CHIP (but never ended up 
> merging). That way, at least we won't impact the other board, and we 
> can have that default. 

For every alike, SPL-compatible, easily-detectable board groups?

>
> Maxime 
>
> -- 
> Maxime Ripard, Free Electrons 
> Embedded Linux and Kernel engineering 
> http://free-electrons.com 
>
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group. 
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com. 
> For more options, visit https://groups.google.com/d/optout. 

[U-Boot] [linux-sunxi] Re: [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[Maxime Ripard]

On Tue, Jan 24, 2017 at 02:30:24PM +0800, Icenowy Zheng wrote:
> 
> 2017?1?24? 01:29? Maxime Ripard <maxime.ripard@free-electrons.com>???
> >
> > On Sat, Jan 21, 2017 at 03:15:27PM +0000, Andr? Przywara wrote: 
> > > >>> On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:? 
> > > >>>> For a board or platform to support FIT loading in the SPL, it has to 
> > > >>>> provide a board_fit_config_name_match() routine, which helps to select 
> > > >>>> one of possibly multiple DTBs contained in a FIT image. 
> > > >>>> Provide a simple function to cover the two different Pine64 models, 
> > > >>>> which can be easily told apart by looking at the amount of installed 
> > > >>>> RAM. 
> > > >>>> 
> > > >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> 
> > > >>>> --- 
> > > >>>>? board/sunxi/board.c | 13 +++++++++++++ 
> > > >>>>? 1 file changed, 13 insertions(+) 
> > > >>>> 
> > > >>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c 
> > > >>>> index 5365638..bbbb826 100644 
> > > >>>> --- a/board/sunxi/board.c 
> > > >>>> +++ b/board/sunxi/board.c 
> > > >>>> @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd) 
> > > >>>>? #endif 
> > > >>>>? return 0; 
> > > >>>>? } 
> > > >>>> + 
> > > >>>> +#ifdef CONFIG_SPL_LOAD_FIT 
> > > >>>> +int board_fit_config_name_match(const char *name) 
> > > >>>> +{ 
> > > >>>> +#ifdef CONFIG_MACH_SUN50I 
> > > >>>> + if ((gd->ram_size > 512 * 1024 * 1024)) 
> > > >>>> + return !strcmp(name, "sun50i-a64-pine64-plus"); 
> > > >>>> + else 
> > > >>>> + return !strcmp(name, "sun50i-a64-pine64"); 
> > > >>>> +#endif 
> > > >>>> + return -1; 
> > > >>>> +}? 
> > > >>> 
> > > >>> That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT 
> > > >>> enabled will be considered a pine64 board?? 
> > > >> 
> > > >> Yes, at least for now, because that's the only A64 board we officially 
> > > >> support so far. 
> > > >> And yes again, it is fishy. It is more a demo or a placeholder for now, 
> > > >> because you _need_ an implementation of board_fit_config_name_match() 
> > > >> once you enable CONFIG_SPL_LOAD_FIT. 
> > > >> 
> > > >> One solution would be to have only one DTB supported per build, so 
> > > >> board_fit_config_name_match() always returns 0. 
> > > >> 
> > > >> Another (better) solution would be to store the board name in the SPL 
> > > >> header, which could be done later when flashing the image. Then this 
> > > >> function would just return strcmp(name, spl_header_name) or 0 if no name 
> > > >> is found for whatever reason. 
> > > > 
> > > > Yes, this is a good solution in the case if we have some non-removable 
> > > > storage on the board (SPI NOR flash, NAND, EEPROM or something else). 
> > > > We can discuss it separately. 
> > > 
> > > I totally agree. I rebased your patch to latest mainline already and 
> > > will send out something later. 
> > > 
> > > > But the current generation of Pine64 boards don't have any 
> > > > non-removable storage yet. My understanding is that you tried to 
> > > > provide the DTB selection, which is based on circumstantial evidences, 
> > > > such as the RAM size. IMHO this is also a valid selection method, 
> > > > because it can reduce the number of required OS image variants. 
> > > 
> > > Yes, the point is that for U-Boot's purposes the only difference between 
> > > the Pine64 and Pine64+ (apart from DRAM size, which is autodetected) is 
> > > using MII vs. RGMII for the Ethernet PHY. The sun8i_emac driver gets 
> > > this information from the DT only, so there is no point at all for 
> > > different defconfigs. And the DRAM size is a safe indicator for that 
> > > difference, at least if we confine our view to Pine64 boards. 
> > > 
> > > Now how other boards fit in here is a separate discussion IMO, so let's 
> > > solve one problem after the other. 
> > > I just thought that we could use: 
> > > 
> > > #ifdef CONFIG_SPL_LOAD_FIT 
> > > int board_fit_config_name_match(const char *name) 
> > > { 
> > > return strcmp(name, CONFIG_DEFAULT_DEVICE_TREE); 
> > > } 
> > > #endif 
> >
> > I guess an easy way around this would be to add a Kconfig option for 
> > the pine64, like I tried to do for the CHIP (but never ended up 
> > merging). That way, at least we won't impact the other board, and we 
> > can have that default. 
> 
> For every alike, SPL-compatible, easily-detectable board groups?

Well this is precisely to be able to do that. If you have another
serie of boards with one flavour having 512MB and the other one having
more, you would have fallen in that case, while it might not be the DT
to pick.

The option prevents you to fall into that hole.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170126/ef77e503/attachment.sig>