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 <[email protected]>
---
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
