Re: [PATCH 11/11] spl: Use CONFIG_IS_ENABLED() for LIBCOMMON_SUPPORT checks

[Quentin Schulz via U-Boot <u-boot@lists.u-boot-project.org>]

Hi Jonas,

On 7/9/26 12:05 AM, Jonas Karlman wrote:
> Use CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT) instead of directly checking
> CONFIG_SPL_LIBCOMMON_SUPPORT when guarding printf/log calls to depend on
> correct xPL_LIBCOMMON_SUPPORT symbol. Also change to use IS_ENABLED()
> for the XPL_BUILD symbol checks for consistency.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>   boot/common_fit.c        |  2 +-
>   common/spl/spl_usb.c     |  2 +-
>   drivers/mmc/mmc-uclass.c |  4 ++--
>   drivers/mmc/mmc.c        | 16 ++++++++--------
>   drivers/mmc/mmc_legacy.c |  4 ++--
>   include/spl.h            |  6 +++---
>   lib/hang.c               |  2 +-
>   7 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/boot/common_fit.c b/boot/common_fit.c
> index fd434fe28e19..9dcec1cd7d7d 100644
> --- a/boot/common_fit.c
> +++ b/boot/common_fit.c
> @@ -53,7 +53,7 @@ int fit_find_config_node(const void *fdt)
>   	     node = fdt_next_subnode(fdt, node)) {
>   		name = fdt_getprop(fdt, node, FIT_DESC_PROP, &len);
>   		if (!name) {
> -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)

I'm stuck on this and cannot make sense of it. I understand we don't 
have a CONFIG_LIBCOMMON_SUPPORT and we may want to printf in proper 
anyway...

I guess the issue is that if we use 
CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT), we will not check for 
CONFIG_SPL_LIBCOMMON_SUPPORT in proper (which is incorrect but what 
we're doing today) anymore and thus the printf won't be compiled in.

I'm thinking the right approach would be to define a

config LIBCOMMON_SUPPORT
     def_bool y
     help
       Enable support for common U-Boot libraries. [...]

and replace all

#if !defined(CONFIG_XPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)

with

#if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)

I'm assuming we don't have an issue with TPL using SPL_LIBCOMMON_SUPPORT 
without TPL_LIBCOMMON_SUPPORT as it won't be able to link due to missing 
library support in TPL?

Then we can simply replace the above in boot/common_fit.c with 
CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT).

We may have an issue with
libs-$(CONFIG_$(PHASE_)LIBCOMMON_SUPPORT) += boot/ common/ cmd/ env/
in scripts/Makefile.xpl as those directories will already be in libs-y 
via the root Makefile for proper?

>   			printf("%s: Missing FDT description in DTB\n",
>   			       __func__);
>   #endif
> diff --git a/common/spl/spl_usb.c b/common/spl/spl_usb.c
> index 932da56ab6db..365d57063c9d 100644
> --- a/common/spl/spl_usb.c
> +++ b/common/spl/spl_usb.c
> @@ -31,7 +31,7 @@ int spl_usb_load(struct spl_image_info *spl_image,
>   	}
>   
>   	if (err) {
> -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> +#if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   		printf("%s: usb init failed: err - %d\n", __func__, err);
>   #endif
>   		return err;
> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
> index 698530088fe7..ffb5e9800fb5 100644
> --- a/drivers/mmc/mmc-uclass.c
> +++ b/drivers/mmc/mmc-uclass.c
> @@ -318,7 +318,7 @@ struct mmc *find_mmc_device(int dev_num)
>   	ret = blk_find_device(UCLASS_MMC, dev_num, &dev);
>   
>   	if (ret) {
> -#if !defined(CONFIG_XPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   		printf("MMC Device %d not found\n", dev_num);
>   #endif
>   		return NULL;
> @@ -390,7 +390,7 @@ void mmc_do_preinit(void)
>   	}
>   }
>   
> -#if !defined(CONFIG_XPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   void print_mmc_devices(char separator)
>   {
>   	struct udevice *dev;
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 2e5655606564..f24a66c389a9 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -328,7 +328,7 @@ int mmc_poll_for_busy(struct mmc *mmc, int timeout_ms)
>   			break;
>   
>   		if (status & MMC_STATUS_MASK) {
> -#if !defined(CONFIG_XPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   			log_err("Status Error: %#08x\n", status);
>   #endif
>   			return -ECOMM;
> @@ -341,7 +341,7 @@ int mmc_poll_for_busy(struct mmc *mmc, int timeout_ms)
>   	}
>   
>   	if (timeout_ms <= 0) {
> -#if !defined(CONFIG_XPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   		log_err("Timeout waiting card ready\n");
>   #endif
>   		return -ETIMEDOUT;
> @@ -483,7 +483,7 @@ static int mmc_read_blocks(struct mmc *mmc, void *dst, lbaint_t start,
>   
>   	if (blkcnt > 1) {
>   		if (mmc_send_stop_transmission(mmc, false)) {
> -#if !defined(CONFIG_XPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   			log_err("mmc fail to send stop cmd\n");
>   #endif
>   			return 0;
> @@ -534,7 +534,7 @@ ulong mmc_bread(struct blk_desc *block_dev, lbaint_t start, lbaint_t blkcnt,
>   		return 0;
>   
>   	if ((start + blkcnt) > block_dev->lba) {
> -#if !defined(CONFIG_XPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   		log_err("MMC: block number 0x" LBAF " exceeds max(0x" LBAF ")\n",
>   			start + blkcnt, block_dev->lba);
>   #endif
> @@ -2769,8 +2769,8 @@ static int mmc_startup(struct mmc *mmc)
>   	bdesc->blksz = mmc->read_bl_len;
>   	bdesc->log2blksz = LOG2(bdesc->blksz);
>   	bdesc->lba = lldiv(mmc->capacity, mmc->read_bl_len);
> -#if !defined(CONFIG_XPL_BUILD) || \
> -		(defined(CONFIG_SPL_LIBCOMMON_SUPPORT) && \
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || \
> +		(CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT) && \
>   		!CONFIG_IS_ENABLED(USE_TINY_PRINTF))

If we also add a
config USE_TINY_PRINTF
     def_bool n
I think we can remove the CONFIG_XPL_BUILD check since 
CONFIG_LIBCOMMON_SUPPORT and CONFIG_USE_TINY_PRINTF exist in proper?

>   	sprintf(bdesc->vendor, "Man %06x Snr %04x%04x",
>   		mmc->cid[0] >> 24, (mmc->cid[2] & 0xffff),
> @@ -3028,7 +3028,7 @@ retry:
>   		err = mmc_send_op_cond(mmc);
>   
>   		if (err) {
> -#if !defined(CONFIG_XPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   			if (!quiet)
>   				log_err("Card did not respond to voltage select! : %d\n",
>   					err);
> @@ -3083,7 +3083,7 @@ int mmc_start_init(struct mmc *mmc)
>   #endif
>   	if (no_card) {
>   		mmc->has_init = 0;
> -#if !defined(CONFIG_XPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   		log_err("MMC: no card present\n");
>   #endif
>   		return -ENOMEDIUM;
> diff --git a/drivers/mmc/mmc_legacy.c b/drivers/mmc/mmc_legacy.c
> index 8f8ba34be717..d29c611011bf 100644
> --- a/drivers/mmc/mmc_legacy.c
> +++ b/drivers/mmc/mmc_legacy.c
> @@ -44,7 +44,7 @@ struct mmc *find_mmc_device(int dev_num)
>   			return m;
>   	}
>   
> -#if !defined(CONFIG_XPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   	printf("MMC Device %d not found\n", dev_num);
>   #endif
>   
> @@ -93,7 +93,7 @@ void mmc_list_add(struct mmc *mmc)
>   	list_add_tail(&mmc->link, &mmc_devices);
>   }
>   
> -#if !defined(CONFIG_XPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   void print_mmc_devices(char separator)
>   {
>   	struct mmc *m;
> diff --git a/include/spl.h b/include/spl.h
> index 45dd44e03d13..98d1a8fda409 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -792,7 +792,7 @@ struct spl_boot_device {
>    * @load_image: Function to call to load image
>    */
>   struct spl_image_loader {
> -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> +#if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   	const char *name;
>   #endif
>   	uint boot_device;
> @@ -809,7 +809,7 @@ struct spl_image_loader {
>   /* Helper function for accessing the name */
>   static inline const char *spl_loader_name(const struct spl_image_loader *loader)
>   {
> -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> +#if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   	const char *name;
>   	name = spl_board_loader_name(loader->boot_device);
>   	return name ?: loader->name;
> @@ -828,7 +828,7 @@ static inline const char *spl_loader_name(const struct spl_image_loader *loader)
>    * _boot_device is the BOOT_DEVICE_... value
>    * _method is the load_image function to call
>    */
> -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> +#if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
>   #define SPL_LOAD_IMAGE_METHOD(_name, _priority, _boot_device, _method) \
>   	SPL_LOAD_IMAGE(_boot_device ## _priority ## _method) = { \
>   		.name = _name, \
> diff --git a/lib/hang.c b/lib/hang.c
> index f3c3c896f0ab..42c5722740a3 100644
> --- a/lib/hang.c
> +++ b/lib/hang.c
> @@ -22,7 +22,7 @@
>    */
>   void hang(void)
>   {
> -#if !defined(CONFIG_XPL_BUILD) || \
> +#if !IS_ENABLED(CONFIG_XPL_BUILD) || \
>   		(CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT) && \
>   		 CONFIG_IS_ENABLED(SERIAL))

If we add a CONFIG_LIBCOMMON_SUPPORT, I think we can remove the 
CONFIG_XPL_BUILD check since CONFIG_LIBCOMMON_SUPPORT and CONFIG_SERIAL 
exist in proper? I do think this would help make things clearer in 
general. What do you think?

A bit of a tangent, one of my biggest gripes about U-Boot is that we do 
not differentiate symbols that are meant to be phase-agnostic and those 
which are specific to a phase. I'm thinking we should have a PROPER_ 
prefix for symbols that are expected to apply to U-Boot proper only. If 
you have neither xPL or PROPER prefix, then it's meant to apply to all 
phases. But we often have mixes and it's difficult to know what's 
supposed to be used or not. I think it would make things much clearer as 
well.

For example, we have some ifdeffery in lib/Makefile around 
USE_TINY_PRINTF which we wouldn't need, had we a symbol for the proper 
phase (which can always be n/y and non-selectable by the user), then we 
simply can remove a bunch of ifeq ($(CONFIG_XPL_BUILD),y) checks.

Anyway, nothing required for this series and likely a big rework that 
may not be worth it in the end.

Cheers,
Quentin