Hi Caleb,
On Fri, 9 Aug 2024 at 11:00, Caleb Connolly <caleb.conno...@linaro.org> wrote:
>
> Hi Simon,
>
> On 09/08/2024 03:55, Simon Glass wrote:
> > Hi Caleb,
> >
> > On Thu, 8 Aug 2024 at 10:32, Caleb Connolly <caleb.conno...@linaro.org
> > <mailto:caleb.conno...@linaro.org>> wrote:
> > >
> > > While U-Boot does a pretty good job at printing information at startup
> > > about what hardware it's running on, it can be hard to take pretty
> > > pictures of this to show off on the internet (by far the most
> > > important aspect of porting a device in 2024!).
> > >
> > > Add a small utility "ufetch" for displaying some information about
> > U-Boot and
> > > the hardware it's running on in a similar fashion to the popular
> > neofetch tool
> > > for *nix's [1].
> > >
> > > While the output is meant to be useful, it should also be pleasing to
> > look at
> > > and look good in screenshots. The ufetch command brings this to U-Boot,
> > > featuring a colorful ASCII art version of the U-Boot logo and a fancy
> > layout.
> > >
> > > Finally, tireless device porters can properly show off their hard
> > work and get
> > > the internet points they so deserve!
> > >
> > > Not everything is fully supported yet, but this seemed good enough
> > for initial
> > > inclusion. Only one MMC device is detected, and other than SCSI other
> > storage
> > > devices aren't supported.
> > >
> > > [1]: https://en.wikipedia.org/wiki/Neofetch
> > <https://en.wikipedia.org/wiki/Neofetch>
> > >
> > > Signed-off-by: Caleb Connolly <caleb.conno...@linaro.org
> > <mailto:caleb.conno...@linaro.org>>
> > > ---
> > > Ephemeral demo image: https://0x0.st/XVUa.png <https://0x0.st/XVUa.png>
> > > ---
> > > cmd/Kconfig | 7 ++
> > > cmd/Makefile | 1 +
> > > cmd/ufetch.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 209 insertions(+)
> > > create mode 100644 cmd/ufetch.c
> >
> > Very cute!
> >
> > >
> > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > index 978f44eda426..70acb5727b04 100644
> > > --- a/cmd/Kconfig
> > > +++ b/cmd/Kconfig
> > > @@ -175,8 +175,15 @@ config CMD_CPU
> > > number of CPUs, type (e.g. manufacturer, architecture,
> > product or
> > > internal name) and clock frequency. Other information may be
> > > available depending on the CPU driver.
> > >
> > > +config CMD_UFETCH
> > > + bool "U-Boot fetch"
> >
> > How about default y if SANDBOX so that this gets built for that. You
> > should also add doc/ and test/ for the command (although the test can be
> > very basic).
>
> I was really aiming for cmd/2048.c levels of expectation and usability
> here, not to make this a rugged part of U-Boot's feature set (if someone
> else would like to go the extra mile they're certainly welcome to).
>
> Making it default y for sandbox so we can at least make sure it compiles
> sounds reasonable though.
> >
> > > + 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.
> > > +
> > > config CMD_FWU_METADATA
> > > bool "fwu metadata read"
> > > depends on FWU_MULTI_BANK_UPDATE
> > > help
> > > diff --git a/cmd/Makefile b/cmd/Makefile
> > > index 87133cc27a8a..ffb04c8f2e0a 100644
> > > --- a/cmd/Makefile
> > > +++ b/cmd/Makefile
> > > @@ -53,8 +53,9 @@ obj-$(CONFIG_CMD_CONSOLE) += console.o
> > > obj-$(CONFIG_CMD_CPU) += cpu.o
> > > obj-$(CONFIG_CMD_DATE) += date.o
> > > obj-$(CONFIG_CMD_DEMO) += demo.o
> > > obj-$(CONFIG_CMD_DM) += dm.o
> > > +obj-$(CONFIG_CMD_UFETCH) += ufetch.o
> > > obj-$(CONFIG_CMD_SOUND) += sound.o
> > > ifdef CONFIG_POST
> > > obj-$(CONFIG_CMD_DIAG) += diag.o
> > > endif
> > > diff --git a/cmd/ufetch.c b/cmd/ufetch.c
> > > new file mode 100644
> > > index 000000000000..f7374eb2e364
> > > --- /dev/null
> > > +++ b/cmd/ufetch.c
> > > @@ -0,0 +1,201 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +/* Small "fetch" utility for U-Boot */
> >
> > Isn't it a command, rather than a utility? I think of a utility as a
> > program I run.
>
> I feel like you could ask 5 different people about this and get 5
> different answers. As a native british english speaker I would view
> "utilities" as a subset of "commands" in this context. This isn't a high
> effort contribution for me so I'm really fine with whatever.
OK I see, fair enough.
> >
> > > +
> > > +#include <mmc.h>
> > > +#include <time.h>
> > > +#include <asm/global_data.h>
> > > +#include <cli.h>
> > > +#include <command.h>
> > > +#include <dm/ofnode.h>
> > > +#include <env.h>
> > > +#include <rand.h>
> > > +#include <vsprintf.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/kernel.h>
> > > +#include <version.h>
> >
> > Please check [1]
>
> Sure, I'll order these alphabetically.
> >
> > > +
> > > +DECLARE_GLOBAL_DATA_PTR;
> > > +
> > > +#define LINE_WIDTH 40
> > > +#define BLUE "\033[38;5;4m"
> > > +#define YELLOW "\033[38;5;11m"
> > > +#define BOLD "\033[1m"
> > > +#define RESET "\033[0m"
> > > +static const char *logo_lines[] = {
> > > + BLUE BOLD " ......::...... ",
> > > + BLUE BOLD " ...::::::::::::::::::... ",
> > > + BLUE BOLD " ..::::::::::::::::::::::::::.. ",
> > > + BLUE BOLD " .::::.:::::::::::::::...::::.::::. ",
> > > + BLUE BOLD " .::::::::::::::::::::..::::::::::::::. ",
> > > + BLUE BOLD " .::.:::::::::::::::::::" YELLOW "=*%#*" BLUE
> > "::::::::::.::. ",
> > > + BLUE BOLD " .:::::::::::::::::....." YELLOW "*%%*-" BLUE
> > ":....::::::::::. ",
> > > + BLUE BOLD " .:.:::...:::::::::.:-" YELLOW "===##*---==-"
> > BLUE "::::::::::.:. ",
> > > + BLUE BOLD " .::::..::::........" YELLOW "-***#****###****-"
> > BLUE "...::::::.:. ",
> > > + BLUE BOLD " ::.:.-" YELLOW "+***+=" BLUE "::-" YELLOW
> > "=+**#%%%%%%%%%%%%###*= " BLUE "-::...::::. ",
> > > + BLUE BOLD ".:.::-" YELLOW
> > "*****###%%%%%%%%%%%%%%%%%%%%%%%%%%#*=" BLUE ":..:::: ",
> > > + BLUE BOLD ".::" YELLOW "##" BLUE ":" YELLOW
> > "***#%%%%%%#####%%%%%%%####%%%%%####%%%*" BLUE "-.::. ",
> > > + BLUE BOLD ":.:" YELLOW "#%" BLUE "::" YELLOW
> > "*%%%%%%%#*****##%%%#*****##%%##*****#%%+" BLUE ".::.",
> > > + BLUE BOLD ".::" YELLOW
> > "**==#%%%%%%%##****#%%%%##****#%%%%#****###%%" BLUE ":.. ",
> > > + BLUE BOLD "..:" YELLOW "#%" BLUE "::" YELLOW
> > "*%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%#%%%%%+ " BLUE ".:.",
> > > + BLUE BOLD " ::" YELLOW "##" BLUE ":" YELLOW
> > "+**#%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%* " BLUE "-.:: ",
> > > + BLUE BOLD " ..::-" YELLOW
> > "#****#%#%%%%%%%%%%%%%%%%%%%%%%%%%%#*=" BLUE "-..::. ",
> > > + BLUE BOLD " ...:=" YELLOW "*****=" BLUE
> > "::-\033[38;5;7m=+**###%%%%%%%%###**+= " BLUE "--:...::: ",
> > > + BLUE BOLD " .::.::--:........::::::--::::::......::::::. ",
> > > + BLUE BOLD " .::.....::::::::::...........:::::::::.::. ",
> > > + BLUE BOLD " .::::::::::::::::::::::::::::::::::::. ",
> > > + BLUE BOLD " .::::.::::::::::::::::::::::.::::. ",
> > > + BLUE BOLD " ..::::::::::::::::::::::::::.. ",
> > > + BLUE BOLD " ...::::::::::::::::::... ",
> > > + BLUE BOLD " ......::...... ",
> > > +};
> > > +
> > > +enum output_lines {
> > > + FIRST,
> > > + SECOND,
> > > + KERNEL,
> > > + SYSINFO,
> > > + HOST,
> > > + UPTIME,
> > > + IP,
> > > + CMDS,
> > > + CONSOLES,
> > > + DEVICES,
> > > + FEATURES,
> > > + RELOCATION,
> > > + CORES,
> > > + MEMORY,
> > > + STORAGE,
> > > +
> > > + /* Up to 10 storage devices... Should be enough for anyone
> > right? */
> > > + _LAST_LINE = (STORAGE + 10),
> > > +#define LAST_LINE (_LAST_LINE - 1UL)
> > > +};
> > > +
> > > +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));
> > > + const char *model, *compatible;
> > > + char *ipaddr;
> > > + int n_cmds, n_cpus = 0, ret, compatlen;
> > > + ofnode np;
> > > + struct mmc *mmc;
> > > + struct blk_desc *scsi_desc;
> > > + bool skip_ascii = false;
> > > +
> > > + if (argc > 1 && strcmp(argv[1], "-n") == 0) {
> > > + skip_ascii = true;
> > > + num_lines = LAST_LINE;
> > > + }
> > > +
> > > + for (int i = 0; i < num_lines; i++) {
> > > + if (!skip_ascii) {
> > > + if (i < ARRAY_SIZE(logo_lines))
> > > + printf("%s ", logo_lines[i]);
> > > + else
> > > + printf("%*c ", LINE_WIDTH, ' ');
> > > + }
> > > + switch (i) {
> > > + case FIRST:
> > > + compatible =
> > ofnode_read_string(ofnode_root(), "compatible");
> > > + if (!compatible)
> > > + compatible = "unknown";
> > > + printf(RESET "%s\n", compatible);
> > > + compatlen = strlen(compatible);
> > > + break;
> > > + case SECOND:
> > > + for (int j = 0; j < compatlen; j++)
> > > + putc('-');
> > > + putc('\n');
> > > + break;
> > > + case KERNEL:
> > > + printf("Kernel:" RESET " %s\n", U_BOOT_VERSION);
> > > + break;
> > > + case SYSINFO:
> > > + printf("Config:" RESET " %s_defconfig\n",
> > CONFIG_SYS_CONFIG_NAME);
> > > + break;
> > > + case HOST:
> > > + model = ofnode_read_string(ofnode_root(),
> > "model");
> > > + if (model)
> > > + printf("Host:" RESET " %s\n", model);
> > > + break;
> > > + case UPTIME:
> > > + printf("Uptime:" RESET " %ld seconds\n",
> > get_timer(0) / 1000);
> > > + break;
> > > + case IP:
> > > + ipaddr = env_get("ipvaddr");
> > > + if (!ipaddr)
> > > + ipaddr = "none";
> > > + printf("IP Address:" RESET " %s", ipaddr);
> > > + ipaddr = env_get("ipv6addr");
> > > + if (ipaddr)
> > > + printf(", %s\n", ipaddr);
> > > + else
> > > + putc('\n');
> > > + break;
> > > + case CMDS:
> > > + n_cmds = ll_entry_count(struct cmd_tbl, cmd);
> > > + printf("Commands:" RESET " %d (help)\n", n_cmds);
> > > + break;
> > > + case CONSOLES:
> > > + printf("Consoles:" RESET " %s (%d baud)\n",
> > env_get("stdout"),
> > > + gd->baudrate);
> > > + break;
> > > + case DEVICES:
> > > + printf("Devices:" RESET " ");
> > > + /* TODO: walk the DM tree */
> > > + printf("Uncountable!\n");
> >
> > See dm_announce() for how to do that.
> >
> > > + break;
> > > + case FEATURES:
> > > + printf("Features:" RESET " ");
> > > + if (IS_ENABLED(CONFIG_NET))
> > > + printf("Net");
> > > + if (IS_ENABLED(CONFIG_EFI_LOADER))
> > > + printf(", EFI");
> >
> > How about EFI_LOADER ? After all, U-Boot can run as an EFI app so 'EFI'
> > might be better reserved for that.
> >
> > > + printf("\n");
> > > + break;
> > > + case RELOCATION:
> > > + if (gd->flags & GD_FLG_SKIP_RELOC)
> > > + printf("Relocated:" RESET " no\n");
> > > + else
> > > + printf("Relocated:" RESET " to
> > %#09lx\n", gd->relocaddr);
> >
> > Not for this patch but I'd really like to figure out a way to
> > enable/disable ANSI codes globally in U-Boot. For example, we should
> > disable them in tests, or when redirecting sandbox to a file. It affects
> > EFI tests too...so if you have any ideas... :-)
>
> Only thing that comes to mind would be adding a compile time flag to
> printf which teaches it to parse and skip ANSI escape codes.
Ideally it would have the option to be run-time, if we want to pay an
extra penalty. Then we can use the same sandbox build with tests as we
do for normal usage.
> >
> > > + break;
> > > + case CORES:
> > > + ofnode_for_each_subnode(np,
> > ofnode_path("/cpus")) {
> > > + if (ofnode_name_eq(np, "cpu"))
> > > + n_cpus++;
> > > + }
> >
> > uclass_id_count(UCLASS_CPU)
> >
> > > + printf("CPU:" RESET " %d (1 in use)\n", n_cpus);
> > > + break;
> > > + case MEMORY:
> > > + printf("Memory:" RESET " %lu MB\n",
> > (ulong)gd->ram_size >> 20);
> > > + break;
> > > + case STORAGE:
> > > + default:
> > > + if (i == STORAGE && get_mmc_num() > 0) {
> > > + mmc = find_mmc_device(0);
> > > + if (mmc)
> > > + printf("Storage:" RESET "
> > mmc 0: %llu MB", mmc->capacity >> 20);
> > > + }
> >
> > How about iterating through UCLASS_BLK ?
>
> Probably a much more sensible approach tbh, can you easily determine the
> size of a block device?
You can do something like this:
struct udevice *dev; // block device
struct blk_desc *desc = dev_get_uclass_plat(dev);
size = desc->lba * desc->blksz
> >
> > > + if (i >= STORAGE + 1) {
> > > + ret = blk_get_desc(UCLASS_SCSI, i -
> > (STORAGE + 1), &scsi_desc);
> > > + if (!ret && scsi_desc->type !=
> > DEV_TYPE_UNKNOWN)
> > > + /* The calculation here is
> > really lossy but close enough */
> > > + printf("Storage:" RESET "
> > scsi %d: %lu MB", i - (STORAGE + 1),
> > > + ((scsi_desc->lba >> 9)
> > * scsi_desc->blksz) >> 11);
> >
> > You can use print_size() - and same above I suppose
>
> haha yeahp i should really do that instead.
>
> Thanks for the review!
> >
> > > + }
> > > + printf("\n");
> > > + }
> > > + }
> > > +
> > > + printf(RESET "\n\n");
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +U_BOOT_CMD(ufetch, 2, 1, do_ufetch,
> > > + "U-Boot fetch utility",
> > > + "Print information about your device.\n"
> > > + " -n Don't print the ASCII logo"
> > > +);
> > > --
> > > 2.46.0
> >
> >
> > [1]
> > https://docs.u-boot.org/en/latest/develop/codingstyle.html#include-files
> > <https://docs.u-boot.org/en/latest/develop/codingstyle.html#include-files>
Regards,
Simon
