diff mbox series

[2/3] mksunxi_fit_atf.sh: Update FIT component descriptions

Message ID 20200507232035.31892-2-samuel@sholland.org
State New
Headers show
Series [1/3] spl: fit: Minimally parse OS properties with FIT_IMAGE_TINY | expand

Commit Message

Samuel Holland May 7, 2020, 11:20 p.m. UTC
Since commit d879616e9e64 ("spl: fit: simplify logic for FDT loading for
non-OS boots"), the SPL looks at the "os" properties of FIT images to
determine where to append the FDT.

The "os" property of the "firmware" image also determines how to execute
the next stage of the boot process, as in 1d3790905d9c ("spl: atf:
introduce spl_invoke_atf and make bl31_entry private").

To support this additional functionality, and to properly model the boot
process, where ATF runs before U-Boot, add the "os" properties and swap
the firmware/loadable images in the FIT image.

Signed-off-by: Samuel Holland <samuel at sholland.org>
---
 board/sunxi/mksunxi_fit_atf.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Patrick Wildt May 8, 2020, 9:45 a.m. UTC | #1
Hi,

now this really confuses me.

commit 0db0ba6141f402b1d496ef53d9fa69978f75ec61 has explicitly made
u-boot the firmware and moved atf into the loadables on NXP i.MX.
Here you do the complete opposite for sunxi.

Can people please make up their minds how it is *supposed* to work?

Oh, and your previous diff about the "minimal os parsing", I need that
too for my use-case, so I like that one!

Patrick

On Thu, May 07, 2020 at 06:20:34PM -0500, Samuel Holland wrote:
> Since commit d879616e9e64 ("spl: fit: simplify logic for FDT loading for
> non-OS boots"), the SPL looks at the "os" properties of FIT images to
> determine where to append the FDT.
> 
> The "os" property of the "firmware" image also determines how to execute
> the next stage of the boot process, as in 1d3790905d9c ("spl: atf:
> introduce spl_invoke_atf and make bl31_entry private").
> 
> To support this additional functionality, and to properly model the boot
> process, where ATF runs before U-Boot, add the "os" properties and swap
> the firmware/loadable images in the FIT image.
> 
> Signed-off-by: Samuel Holland <samuel at sholland.org>
> ---
>  board/sunxi/mksunxi_fit_atf.sh | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/board/sunxi/mksunxi_fit_atf.sh b/board/sunxi/mksunxi_fit_atf.sh
> index 88ad719747..4dfd22db78 100755
> --- a/board/sunxi/mksunxi_fit_atf.sh
> +++ b/board/sunxi/mksunxi_fit_atf.sh
> @@ -31,6 +31,7 @@ cat << __HEADER_EOF
>  			description = "U-Boot (64-bit)";
>  			data = /incbin/("u-boot-nodtb.bin");
>  			type = "standalone";
> +			os = "u-boot";
>  			arch = "arm64";
>  			compression = "none";
>  			load = <0x4a000000>;
> @@ -39,6 +40,7 @@ cat << __HEADER_EOF
>  			description = "ARM Trusted Firmware";
>  			data = /incbin/("$BL31");
>  			type = "firmware";
> +			os = "arm-trusted-firmware";
>  			arch = "arm64";
>  			compression = "none";
>  			load = <$BL31_ADDR>;
> @@ -73,8 +75,8 @@ do
>  	cat << __CONF_SECTION_EOF
>  		config_$cnt {
>  			description = "$(basename $dtname .dtb)";
> -			firmware = "uboot";
> -			loadables = "atf";
> +			firmware = "atf";
> +			loadables = "uboot";
>  			fdt = "fdt_$cnt";
>  		};
>  __CONF_SECTION_EOF
> -- 
> 2.24.1
>
Samuel Holland May 9, 2020, 7:02 p.m. UTC | #2
Hi,

On 5/8/20 4:45 AM, Patrick Wildt wrote:
> Hi,
> 
> now this really confuses me.
> 
> commit 0db0ba6141f402b1d496ef53d9fa69978f75ec61 has explicitly made
> u-boot the firmware and moved atf into the loadables on NXP i.MX.
> Here you do the complete opposite for sunxi.
> 
> Can people please make up their minds how it is *supposed* to work?

I don't think that commit is suggesting how things are supposed to work; it's a
workaround responding to the existing limitations in SPL_FIT_IMAGE_TINY.
Specifically, that "firmware" is assumed to be U-Boot, and "loadables" are
assumed to be something else.

The first patch in this series removes those limitations by actually looking at
the "os" property. With my first patch applied, U-Boot would be detected in
either list, so booting would work with or without commit 0db0ba6141f4.

So for the reasons I outline below (the functionality of the "switch
(spl_image.os)" in board_init_r), it might make sense to revert that commit
after applying this series.

Cheers,
Samuel

> Oh, and your previous diff about the "minimal os parsing", I need that
> too for my use-case, so I like that one!
> 
> Patrick
> 
> On Thu, May 07, 2020 at 06:20:34PM -0500, Samuel Holland wrote:
>> Since commit d879616e9e64 ("spl: fit: simplify logic for FDT loading for
>> non-OS boots"), the SPL looks at the "os" properties of FIT images to
>> determine where to append the FDT.
>>
>> The "os" property of the "firmware" image also determines how to execute
>> the next stage of the boot process, as in 1d3790905d9c ("spl: atf:
>> introduce spl_invoke_atf and make bl31_entry private").
>>
>> To support this additional functionality, and to properly model the boot
>> process, where ATF runs before U-Boot, add the "os" properties and swap
>> the firmware/loadable images in the FIT image.
>>
>> Signed-off-by: Samuel Holland <samuel at sholland.org>
>> ---
>>  board/sunxi/mksunxi_fit_atf.sh | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/board/sunxi/mksunxi_fit_atf.sh b/board/sunxi/mksunxi_fit_atf.sh
>> index 88ad719747..4dfd22db78 100755
>> --- a/board/sunxi/mksunxi_fit_atf.sh
>> +++ b/board/sunxi/mksunxi_fit_atf.sh
>> @@ -31,6 +31,7 @@ cat << __HEADER_EOF
>>  			description = "U-Boot (64-bit)";
>>  			data = /incbin/("u-boot-nodtb.bin");
>>  			type = "standalone";
>> +			os = "u-boot";
>>  			arch = "arm64";
>>  			compression = "none";
>>  			load = <0x4a000000>;
>> @@ -39,6 +40,7 @@ cat << __HEADER_EOF
>>  			description = "ARM Trusted Firmware";
>>  			data = /incbin/("$BL31");
>>  			type = "firmware";
>> +			os = "arm-trusted-firmware";
>>  			arch = "arm64";
>>  			compression = "none";
>>  			load = <$BL31_ADDR>;
>> @@ -73,8 +75,8 @@ do
>>  	cat << __CONF_SECTION_EOF
>>  		config_$cnt {
>>  			description = "$(basename $dtname .dtb)";
>> -			firmware = "uboot";
>> -			loadables = "atf";
>> +			firmware = "atf";
>> +			loadables = "uboot";
>>  			fdt = "fdt_$cnt";
>>  		};
>>  __CONF_SECTION_EOF
>> -- 
>> 2.24.1
>>
Patrick Wildt May 9, 2020, 7:09 p.m. UTC | #3
On Sat, May 09, 2020 at 02:02:19PM -0500, Samuel Holland wrote:
> On 5/8/20 4:45 AM, Patrick Wildt wrote:
> > Hi,
> > 
> > now this really confuses me.
> > 
> > commit 0db0ba6141f402b1d496ef53d9fa69978f75ec61 has explicitly made
> > u-boot the firmware and moved atf into the loadables on NXP i.MX.
> > Here you do the complete opposite for sunxi.
> > 
> > Can people please make up their minds how it is *supposed* to work?
> 
> I don't think that commit is suggesting how things are supposed to work; it's a
> workaround responding to the existing limitations in SPL_FIT_IMAGE_TINY.
> Specifically, that "firmware" is assumed to be U-Boot, and "loadables" are
> assumed to be something else.
> 
> The first patch in this series removes those limitations by actually looking at
> the "os" property. With my first patch applied, U-Boot would be detected in
> either list, so booting would work with or without commit 0db0ba6141f4.
> 
> So for the reasons I outline below (the functionality of the "switch
> (spl_image.os)" in board_init_r), it might make sense to revert that commit
> after applying this series.
> 
> Cheers,
> Samuel

I tend to agree.  Having ATF as a "firmware", spl_load_simple_fit()
would with your diff actually recognize that it's ATF and not U-Boot, so
it wouldn't append the FDT.  Then it goes over the loadables.  Loads
U-Boot, sees it is U-Boot, appends the FDT, and then (possibly) OP-TEE.

I'm still not sure what should be "firmware" and what "loadables", but
if ATF is supposed to be "firmware", your diff makes sense and it would
make sense to revert the change in the i.MX mkimage script.

So in that case I'd say:

Acked-by: Patrick Wildt <patrick at blueri.se>

Best regards,
Patrick
diff mbox series

Patch

diff --git a/board/sunxi/mksunxi_fit_atf.sh b/board/sunxi/mksunxi_fit_atf.sh
index 88ad719747..4dfd22db78 100755
--- a/board/sunxi/mksunxi_fit_atf.sh
+++ b/board/sunxi/mksunxi_fit_atf.sh
@@ -31,6 +31,7 @@  cat << __HEADER_EOF
 			description = "U-Boot (64-bit)";
 			data = /incbin/("u-boot-nodtb.bin");
 			type = "standalone";
+			os = "u-boot";
 			arch = "arm64";
 			compression = "none";
 			load = <0x4a000000>;
@@ -39,6 +40,7 @@  cat << __HEADER_EOF
 			description = "ARM Trusted Firmware";
 			data = /incbin/("$BL31");
 			type = "firmware";
+			os = "arm-trusted-firmware";
 			arch = "arm64";
 			compression = "none";
 			load = <$BL31_ADDR>;
@@ -73,8 +75,8 @@  do
 	cat << __CONF_SECTION_EOF
 		config_$cnt {
 			description = "$(basename $dtname .dtb)";
-			firmware = "uboot";
-			loadables = "atf";
+			firmware = "atf";
+			loadables = "uboot";
 			fdt = "fdt_$cnt";
 		};
 __CONF_SECTION_EOF