[PATCH v5] spl: remove usage of CMD_BOOT[IZ] from image parsing

[PATCH v5] spl: remove usage of CMD_BOOT[IZ] from image parsing

[Anshul Dalal]

Using CMD_* configs from spl doesn't make logical sense. Therefore this
patch replaces the checks for CMD_BOOT[IZ] with newly added configs
SPL_HAS_BOOT[IZ].

SPL_HAS_BOOTZ is enabled by default for 32-bit ARM systems and
SPL_HAS_BOOTI is enabled by default for 64-bit ARM and RISCV. This
ensures configs relying on CMD_BOOT[IZ] in falcon boot still work.

Signed-off-by: Anshul Dalal <anshuld@ti.com>
---
Changes in v5:
 * Remove imply clause for CMD_BOOTZ instead add default y for SPL_HAS_BOOTZ
 * Update commit message to reflect the changes
 * Remove 'More info' link
v4: https://lore.kernel.org/u-boot/20250313032842.1189977-1-anshuld@ti.com/
Changes in v4:
 * Don't set SPL_HAS_BOOTI for sandbox by default
 * Updated prompts for SPL_HAS_BOOT[IZ]
 * Removed check for SPL_HAS_FRAMEWORK from Makefile
v3: https://lore.kernel.org/u-boot/20250312124757.789013-1-anshuld@ti.com/
Changes in v3:
 * Add imply clause for CMD_BOOTZ to enable SPL_HAS_BOOTZ
 * Fix broken check for bootz_setup
v2: https://lore.kernel.org/u-boot/20250312094241.629707-1-anshuld@ti.com/
Changes in v2:
 * Add SPL_HAS_BOOT[IZ] configs
v1: https://lore.kernel.org/u-boot/20250311093709.3372104-1-anshuld@ti.com/
---
 arch/arm/lib/Makefile |  6 ++----
 common/spl/Kconfig    | 14 ++++++++++++++
 common/spl/spl.c      |  5 +++--
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 1c95dd6fed2..e409f7a7947 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -35,10 +35,8 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o
 obj-$(CONFIG_CMD_BOOTZ) += bootm.o zimage.o
 else
 obj-$(CONFIG_$(PHASE_)FRAMEWORK) += spl.o
-ifdef CONFIG_SPL_FRAMEWORK
-obj-$(CONFIG_CMD_BOOTI) += image.o
-obj-$(CONFIG_CMD_BOOTZ) += zimage.o
-endif
+obj-$(CONFIG_SPL_HAS_BOOTI) += image.o
+obj-$(CONFIG_SPL_HAS_BOOTZ) += zimage.o
 obj-$(CONFIG_OF_LIBFDT) += bootm-fdt.o
 endif
 ifdef CONFIG_ARM64
diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index 21a5cefee7a..1d690bee389 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -1153,6 +1153,20 @@ config SPL_OS_BOOT
 	  Enable booting directly to an OS from SPL.
 	  for more info read doc/README.falcon
 
+config SPL_HAS_BOOTZ
+	bool "Allow booting a zImage style Linux kernel from SPL"
+	depends on SPL_OS_BOOT
+	default y if ARM && !ARM64
+	help
+	  Boot a linux zimage from memory in falcon boot.
+
+config SPL_HAS_BOOTI
+	bool "Allow booting an Image style Linux kernel from SPL"
+	depends on SPL_OS_BOOT
+	default y if ARM64 || RISCV
+	help
+	  Boot an uncompressed linux kernel image from memory in falcon boot.
+
 config SPL_PAYLOAD_ARGS_ADDR
 	hex "Address in memory to load 'args' file for Falcon Mode to"
 	depends on SPL_OS_BOOT || SPL_LOAD_FIT_OPENSBI_OS_BOOT
diff --git a/common/spl/spl.c b/common/spl/spl.c
index 76fd56dfe4b..445c3ef24fe 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -335,7 +335,7 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
 		panic("** no mkimage signature but raw image not supported");
 	}
 
-	if (CONFIG_IS_ENABLED(OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI)) {
+	if (CONFIG_IS_ENABLED(OS_BOOT) && IS_ENABLED(CONFIG_SPL_HAS_BOOTI)) {
 		ulong start, size;
 
 		if (!booti_setup((ulong)header, &start, &size, 0)) {
@@ -349,7 +349,8 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
 			      spl_image->load_addr, spl_image->size);
 			return 0;
 		}
-	} else if (CONFIG_IS_ENABLED(OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTZ)) {
+	} else if (CONFIG_IS_ENABLED(OS_BOOT) &&
+		   IS_ENABLED(CONFIG_SPL_HAS_BOOTZ)) {
 		ulong start, end;
 
 		if (!bootz_setup((ulong)header, &start, &end)) {
-- 
2.43.0

Re: [PATCH v5] spl: remove usage of CMD_BOOT[IZ] from image parsing

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On Fri, Mar 14, 2025 at 09:25:04AM +0530, Anshul Dalal wrote:

> Using CMD_* configs from spl doesn't make logical sense. Therefore this
> patch replaces the checks for CMD_BOOT[IZ] with newly added configs
> SPL_HAS_BOOT[IZ].
> 
> SPL_HAS_BOOTZ is enabled by default for 32-bit ARM systems and
> SPL_HAS_BOOTI is enabled by default for 64-bit ARM and RISCV. This
> ensures configs relying on CMD_BOOT[IZ] in falcon boot still work.
> 
> Signed-off-by: Anshul Dalal <anshuld@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v5] spl: remove usage of CMD_BOOT[IZ] from image parsing

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On Fri, Mar 14, 2025 at 09:25:04AM +0530, Anshul Dalal wrote:

> Using CMD_* configs from spl doesn't make logical sense. Therefore this
> patch replaces the checks for CMD_BOOT[IZ] with newly added configs
> SPL_HAS_BOOT[IZ].
> 
> SPL_HAS_BOOTZ is enabled by default for 32-bit ARM systems and
> SPL_HAS_BOOTI is enabled by default for 64-bit ARM and RISCV. This
> ensures configs relying on CMD_BOOT[IZ] in falcon boot still work.
> 
> Signed-off-by: Anshul Dalal <anshuld@ti.com>

OK, so this needs to be introducing some library symbol which then both
CMD_BOOTx and SPL_...something select. What we have here introduces
failure to build on some imx8 platforms such as 
mx8mp_evk and then size growth on others such as imx28_xea.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v5] spl: remove usage of CMD_BOOT[IZ] from image parsing

[Anshul Dalal]

On Tue Apr 1, 2025 at 4:33 AM IST, Tom Rini wrote:
> On Fri, Mar 14, 2025 at 09:25:04AM +0530, Anshul Dalal wrote:
>
>> Using CMD_* configs from spl doesn't make logical sense. Therefore this
>> patch replaces the checks for CMD_BOOT[IZ] with newly added configs
>> SPL_HAS_BOOT[IZ].
>> 
>> SPL_HAS_BOOTZ is enabled by default for 32-bit ARM systems and
>> SPL_HAS_BOOTI is enabled by default for 64-bit ARM and RISCV. This
>> ensures configs relying on CMD_BOOT[IZ] in falcon boot still work.
>> 
>> Signed-off-by: Anshul Dalal <anshuld@ti.com>
>
> OK, so this needs to be introducing some library symbol which then both
> CMD_BOOTx and SPL_...something select.

I was thinking of chaning the Makefile with the following diff:

-obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o
+obj-$(CONFIG_LIB_BOOTI) += image.o
 obj-$(CONFIG_CMD_BOOTM) += bootm.o
-obj-$(CONFIG_CMD_BOOTZ) += bootm.o zimage.o
+obj-$(CONFIG_LIB_BOOTZ) += zimage.o
 else
 obj-$(CONFIG_$(PHASE_)FRAMEWORK) += spl.o
-obj-$(CONFIG_SPL_HAS_BOOTI) += image.o
-obj-$(CONFIG_SPL_HAS_BOOTZ) += zimage.o
 obj-$(CONFIG_OF_LIBFDT) += bootm-fdt.o
 endif

This would simplify the Makefile by not having duplicated configs for
image/zimage and the removed bootm.o for non-spl builds should be fine
since except for colibri_vf, all defconfigs with CMD_BOOTx already have
CMD_BOOTM enabled. And for colibri_vf, we can safely enable CMD_BOOTM?


> What we have here introduces
> failure to build on some imx8 platforms such as 
> mx8mp_evk and then size growth on others such as imx28_xea.

The imx8mp_evk build should not fail since the patch should only effect
falcon mode. Only difference this patch makes is now image.o would not
be compiled for the spl which is not used in non falcon boot anyways.

For the imx28_xea and 3 other (imx6qdl_icore_mipi|mmc|rqs) defconfigs
that use falcon mode but don't make use of CMD_BOOTx. Their size growth
is expected since SPL_HAS_BOOTx is default y in falcon boot. To keep the
same size, the SPL_HAS_BOOTx can be explicitly disabled for those 4
configs.

For future reference is there any CI tests I can run to detect any
regressions before posting patches upstream?

Regards,
Anshul

Re: [PATCH v5] spl: remove usage of CMD_BOOT[IZ] from image parsing

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2837 bytes --]

On Wed, Apr 02, 2025 at 04:00:01PM +0530, Anshul Dalal wrote:
> On Tue Apr 1, 2025 at 4:33 AM IST, Tom Rini wrote:
> > On Fri, Mar 14, 2025 at 09:25:04AM +0530, Anshul Dalal wrote:
> >
> >> Using CMD_* configs from spl doesn't make logical sense. Therefore this
> >> patch replaces the checks for CMD_BOOT[IZ] with newly added configs
> >> SPL_HAS_BOOT[IZ].
> >> 
> >> SPL_HAS_BOOTZ is enabled by default for 32-bit ARM systems and
> >> SPL_HAS_BOOTI is enabled by default for 64-bit ARM and RISCV. This
> >> ensures configs relying on CMD_BOOT[IZ] in falcon boot still work.
> >> 
> >> Signed-off-by: Anshul Dalal <anshuld@ti.com>
> >
> > OK, so this needs to be introducing some library symbol which then both
> > CMD_BOOTx and SPL_...something select.
> 
> I was thinking of chaning the Makefile with the following diff:
> 
> -obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o
> +obj-$(CONFIG_LIB_BOOTI) += image.o
>  obj-$(CONFIG_CMD_BOOTM) += bootm.o
> -obj-$(CONFIG_CMD_BOOTZ) += bootm.o zimage.o
> +obj-$(CONFIG_LIB_BOOTZ) += zimage.o
>  else
>  obj-$(CONFIG_$(PHASE_)FRAMEWORK) += spl.o
> -obj-$(CONFIG_SPL_HAS_BOOTI) += image.o
> -obj-$(CONFIG_SPL_HAS_BOOTZ) += zimage.o
>  obj-$(CONFIG_OF_LIBFDT) += bootm-fdt.o
>  endif
> 
> This would simplify the Makefile by not having duplicated configs for
> image/zimage and the removed bootm.o for non-spl builds should be fine
> since except for colibri_vf, all defconfigs with CMD_BOOTx already have
> CMD_BOOTM enabled. And for colibri_vf, we can safely enable CMD_BOOTM?
> 
> 
> > What we have here introduces
> > failure to build on some imx8 platforms such as 
> > mx8mp_evk and then size growth on others such as imx28_xea.
> 
> The imx8mp_evk build should not fail since the patch should only effect
> falcon mode. Only difference this patch makes is now image.o would not
> be compiled for the spl which is not used in non falcon boot anyways.
> 
> For the imx28_xea and 3 other (imx6qdl_icore_mipi|mmc|rqs) defconfigs
> that use falcon mode but don't make use of CMD_BOOTx. Their size growth
> is expected since SPL_HAS_BOOTx is default y in falcon boot. To keep the
> same size, the SPL_HAS_BOOTx can be explicitly disabled for those 4
> configs.
> 
> For future reference is there any CI tests I can run to detect any
> regressions before posting patches upstream?

To run CI please see
https://docs.u-boot.org/en/latest/develop/ci_testing.html

And yes, please re-work the symbols and logic so there's no size growth
until the K3 R5 platforms enable things, as the default should not
change behavior. Using
https://source.denx.de/u-boot/u-boot-extras/-/blob/master/contrib/trini/u-boot-size-test.sh?ref_type=heads
might make it easier to compare various platforms before/after.

> 
> Regards,
> Anshul

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v5] spl: remove usage of CMD_BOOT[IZ] from image parsing

[Anshul Dalal]

On Fri Mar 14, 2025 at 9:25 AM IST, Anshul Dalal wrote:
> Using CMD_* configs from spl doesn't make logical sense. Therefore this
> patch replaces the checks for CMD_BOOT[IZ] with newly added configs
> SPL_HAS_BOOT[IZ].
>
> SPL_HAS_BOOTZ is enabled by default for 32-bit ARM systems and
> SPL_HAS_BOOTI is enabled by default for 64-bit ARM and RISCV. This
> ensures configs relying on CMD_BOOT[IZ] in falcon boot still work.
>
> Signed-off-by: Anshul Dalal <anshuld@ti.com>
> ---
> Changes in v5:
>  * Remove imply clause for CMD_BOOTZ instead add default y for SPL_HAS_BOOTZ
>  * Update commit message to reflect the changes
>  * Remove 'More info' link
> v4: https://lore.kernel.org/u-boot/20250313032842.1189977-1-anshuld@ti.com/
> Changes in v4:
>  * Don't set SPL_HAS_BOOTI for sandbox by default
>  * Updated prompts for SPL_HAS_BOOT[IZ]
>  * Removed check for SPL_HAS_FRAMEWORK from Makefile
> v3: https://lore.kernel.org/u-boot/20250312124757.789013-1-anshuld@ti.com/
> Changes in v3:
>  * Add imply clause for CMD_BOOTZ to enable SPL_HAS_BOOTZ
>  * Fix broken check for bootz_setup
> v2: https://lore.kernel.org/u-boot/20250312094241.629707-1-anshuld@ti.com/
> Changes in v2:
>  * Add SPL_HAS_BOOT[IZ] configs
> v1: https://lore.kernel.org/u-boot/20250311093709.3372104-1-anshuld@ti.com/
> ---
>  arch/arm/lib/Makefile |  6 ++----
>  common/spl/Kconfig    | 14 ++++++++++++++
>  common/spl/spl.c      |  5 +++--
>  3 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index 1c95dd6fed2..e409f7a7947 100644
> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -35,10 +35,8 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o
>  obj-$(CONFIG_CMD_BOOTZ) += bootm.o zimage.o
>  else
>  obj-$(CONFIG_$(PHASE_)FRAMEWORK) += spl.o
> -ifdef CONFIG_SPL_FRAMEWORK
> -obj-$(CONFIG_CMD_BOOTI) += image.o
> -obj-$(CONFIG_CMD_BOOTZ) += zimage.o
> -endif
> +obj-$(CONFIG_SPL_HAS_BOOTI) += image.o
> +obj-$(CONFIG_SPL_HAS_BOOTZ) += zimage.o
>  obj-$(CONFIG_OF_LIBFDT) += bootm-fdt.o
>  endif
>  ifdef CONFIG_ARM64
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index 21a5cefee7a..1d690bee389 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -1153,6 +1153,20 @@ config SPL_OS_BOOT
>  	  Enable booting directly to an OS from SPL.
>  	  for more info read doc/README.falcon
>  
> +config SPL_HAS_BOOTZ
> +	bool "Allow booting a zImage style Linux kernel from SPL"
> +	depends on SPL_OS_BOOT
> +	default y if ARM && !ARM64
> +	help
> +	  Boot a linux zimage from memory in falcon boot.
> +
> +config SPL_HAS_BOOTI
> +	bool "Allow booting an Image style Linux kernel from SPL"
> +	depends on SPL_OS_BOOT
> +	default y if ARM64 || RISCV
> +	help
> +	  Boot an uncompressed linux kernel image from memory in falcon boot.
> +
>  config SPL_PAYLOAD_ARGS_ADDR
>  	hex "Address in memory to load 'args' file for Falcon Mode to"
>  	depends on SPL_OS_BOOT || SPL_LOAD_FIT_OPENSBI_OS_BOOT
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 76fd56dfe4b..445c3ef24fe 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -335,7 +335,7 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
>  		panic("** no mkimage signature but raw image not supported");
>  	}
>  
> -	if (CONFIG_IS_ENABLED(OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTI)) {
> +	if (CONFIG_IS_ENABLED(OS_BOOT) && IS_ENABLED(CONFIG_SPL_HAS_BOOTI)) {
>  		ulong start, size;
>  
>  		if (!booti_setup((ulong)header, &start, &size, 0)) {
> @@ -349,7 +349,8 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
>  			      spl_image->load_addr, spl_image->size);
>  			return 0;
>  		}
> -	} else if (CONFIG_IS_ENABLED(OS_BOOT) && IS_ENABLED(CONFIG_CMD_BOOTZ)) {
> +	} else if (CONFIG_IS_ENABLED(OS_BOOT) &&
> +		   IS_ENABLED(CONFIG_SPL_HAS_BOOTZ)) {
>  		ulong start, end;
>  
>  		if (!bootz_setup((ulong)header, &start, &end)) {

Superseded by v6:
https://lore.kernel.org/u-boot/20250403215522.1284502-1-anshuld@ti.com/

Anshul