Hi J,

Thanks for the patch!

On 05/12/2024 19:35, J. Neuschäfer via B4 Relay wrote:

From: "J. Neuschäfer" j.ne@posteo.net

On 32-bit architectures, LAST_LINE (_LAST_LINE - 1UL) is 64 bits long, but size_t (from ARRAY_SIZE(...)) is 32 bits. This results in a warning because the max() macro expects the same type on both sides:

cmd/ufetch.c: In function ‘do_ufetch’: include/linux/kernel.h:179:24: warning: comparison of distinct pointer types lacks a cast [-Wcompare-distinct-pointer-types] 179 | (void) (&_max1 == &_max2); \ | ^~ cmd/ufetch.c:92:25: note: in expansion of macro ‘max’ 92 | int num_lines = max(LAST_LINE + 1, ARRAY_SIZE(logo_lines)); | ^~~

Fix this by casting LAST_LINE to size_t.

Signed-off-by: J. Neuschäfer j.ne@posteo.net

Reviewed-by: Caleb Connolly caleb.connolly@linaro.org

---

cmd/ufetch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/ufetch.c b/cmd/ufetch.c index 0b825d7e8c75f3b18933d3e3f77e5f40f2c7b658..5f3ef847b268dc384271fc6774720e5fd2337157 100644 --- a/cmd/ufetch.c +++ b/cmd/ufetch.c @@ -89,7 +89,7 @@ enum output_lines { static int do_ufetch(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) {

• int num_lines = max(LAST_LINE + 1, ARRAY_SIZE(logo_lines));

• int num_lines = max((size_t)LAST_LINE + 1, ARRAY_SIZE(logo_lines)); const char *model, *compatible; char *ipaddr; int n_cmds, n_cpus = 0, ret, compatlen;

On 05/12/2024 19:35, J. Neuschäfer via B4 Relay wrote:

From: "J. Neuschäfer" j.ne@posteo.net

The ufetch command is still quite useful on systems without block device support; remove the CONFIG_BLK dependency and make sure the code compiles/works with and without CONFIG_BLK.

Signed-off-by: J. Neuschäfer j.ne@posteo.net

Small nit below, but with that:

Reviewed-by: Caleb Connolly caleb.connolly@linaro.org

---

cmd/Kconfig | 1 - cmd/ufetch.c | 10 +++++++--- 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig index 4936a70f3ef16ddb093ceafa12d011ca1b89e95c..547fd2a91f7883e2ae5982897ec93c4483d67852 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -178,7 +178,6 @@ config CMD_CPU

config CMD_UFETCH bool "U-Boot fetch"

• depends on BLK help Fetch utility for U-Boot (akin to neofetch). Prints information about U-Boot and the board it is running on in a pleasing format.

diff --git a/cmd/ufetch.c b/cmd/ufetch.c index 5f3ef847b268dc384271fc6774720e5fd2337157..7aed0b447bda104b837d37c6adcbb21b80237aba 100644 --- a/cmd/ufetch.c +++ b/cmd/ufetch.c @@ -92,11 +92,9 @@ static int do_ufetch(struct cmd_tbl *cmdtp, int flag, int argc, int num_lines = max((size_t)LAST_LINE + 1, ARRAY_SIZE(logo_lines)); const char *model, *compatible; char *ipaddr;

• int n_cmds, n_cpus = 0, ret, compatlen;

• int n_cmds, n_cpus = 0, compatlen; size_t size; ofnode np;

• struct udevice *dev;

• struct blk_desc *desc; bool skip_ascii = false;

  if (argc > 1 && strcmp(argv[1], "-n") == 0) {

@@ -200,6 +198,11 @@ static int do_ufetch(struct cmd_tbl *cmdtp, int flag, int argc, break; case STORAGE: default: +#ifdef CONFIG_BLK

• 	struct udevice *dev;
  

At least without the #ifdef, I see a warning "A label followed by a declaration is a C23 extension". So to be on the safe side please put this section in a block:

default: {

...

}

Kind regards,

• 	struct blk_desc *desc;
  

• 	int ret;
  

• ret = uclass_find_device_by_seq(UCLASS_BLK, line - STORAGE, &dev);
  if (!ret && dev) {
  	desc = dev_get_uclass_plat(dev);
  

@@ -213,6 +216,7 @@ static int do_ufetch(struct cmd_tbl *cmdtp, int flag, int argc, } else if (ret == -ENODEV && (skip_ascii || line > ARRAY_SIZE(logo_lines))) { break; } +#endif printf("\n"); } }

Thanks for the patch!

On 05/12/2024 19:35, J. Neuschäfer via B4 Relay wrote:

From: "J. Neuschäfer" j.ne@posteo.net

When looking at ufetch output it isn't immediately obvious which CPU architecture the presented board has. This patch therefore adds the CPU architecture string (for example "powerpc") to the "CPU:" line.

It would be nice to have the proper fancy name, but this is an improvement nonetheless.

Signed-off-by: J. Neuschäfer j.ne@posteo.net

cmd/ufetch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/ufetch.c b/cmd/ufetch.c index 7aed0b447bda104b837d37c6adcbb21b80237aba..0ee50bfb606aa2223072c63cd46b3a70ef5449fd 100644 --- a/cmd/ufetch.c +++ b/cmd/ufetch.c @@ -188,7 +188,7 @@ static int do_ufetch(struct cmd_tbl *cmdtp, int flag, int argc, if (ofnode_name_eq(np, "cpu")) n_cpus++; }

• 	printf("CPU:" RESET " %d (1 in use)\n", n_cpus);
  

• 	printf("CPU: " RESET CONFIG_SYS_ARCH ", %d (1 in use)\n", n_cpus);
  

This will read like

CPU: arm 4 (1 in use)

which is a bit hard to parse at a glance. How about

CPU: arm [4 cores]

or some other separator between SYS_ARCH and the core count?

Kind regards,

	break;
case MEMORY:
	for (int j = 0; j < CONFIG_NR_DRAM_BANKS && gd->bd->bi_dram[j].size; j++)