[PATCH v2 2/5] arm: mach-imx: Add command to expose QB functionality

[Simona Toaca]

On Mon, Mar 16, 2026 at 01:20:06PM +0100, Marek Vasut wrote:
> On 3/16/26 9:15 AM, Simona Toaca (OSS) wrote:
> 
> [...]
> 
> > +++ b/arch/arm/mach-imx/Kconfig
> > @@ -71,6 +71,17 @@ config CSF_SIZE
> >   	  Define the maximum size for Command Sequence File (CSF) binary
> >   	  this information is used to define the image boot data.
> > +config CMD_IMX_QB
> > +	bool "Support the 'qb' command"
> > +	default y
> > +	depends on IMX_SNPS_DDR_PHY_QB_GEN && (IMX95 || IMX94)
> 
> 94 should be before 95
> 

[ ... ]

> > +static struct cmd_tbl cmd_qb[] = {
> > +	U_BOOT_CMD_MKENT(check, 1, 1, do_qb_check, "", ""),
> > +	U_BOOT_CMD_MKENT(save,  3, 1, do_qb_save,  "", ""),
> > +	U_BOOT_CMD_MKENT(erase, 3, 1, do_qb_erase, "", ""),
> > +};
> > +
> > +static int do_qbops(struct cmd_tbl *cmdtp, int flag, int argc,
> > +		    char *const argv[])
> > +{
> > +	struct cmd_tbl *cp;
> > +
> > +	cp = find_cmd_tbl(argv[1], cmd_qb, ARRAY_SIZE(cmd_qb));
> > +
> > +	/* Drop the qb command */
> > +	argc--;
> > +	argv++;
> > +
> > +	if (!cp) {
> > +		printf("%s cp is null\n", __func__);
> 
> What does this error output even mean ? The user will be confused, please
> reword it.
> 

Looking at this, these error messages might be redundant, as U-Boot displays
the command usage anyway if CMD_RET_USAGE is returned. The repeatable error is
also redundant because the command is declared repeatable.

I have a suggestion: I could use the U-Boot API for commands from
include/command.h (U_BOOT_CMD_WITH_SUBCMDS), which declares and registers
do_<cmdname> with the default method body (which is almost the same as this
version). This would imply no actual error messages, just printing the usage,
and a cleaner code.

If it's not a good approach, I will just modify the error message here as you
suggested.

> > +		return CMD_RET_USAGE;
> > +	}
> > +
> > +	if (argc > cp->maxargs) {
> > +		printf("%s argc(%d) > cp->maxargs(%d)\n", __func__, argc, cp->maxargs);
> 
> Reword this too please.
> 
> > +		return CMD_RET_USAGE;
> > +	}
> > +
> > +	if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp)) {
> > +		printf("%s %s repeat flag set  but command is not repeatable\n",
> 
> One space is enough.
> 
> > +		       __func__, cp->name);
> > +		return CMD_RET_SUCCESS;
> > +	}
> > +
> > +	return cp->cmd(cmdtp, flag, argc, argv);
> > +}
> > +
> > +U_BOOT_CMD(
> > +	qb, 4, 1, do_qbops,
> > +	"DDR Quick Boot sub system",
> > +	"check - check if quick boot data is stored in mem by training flow\n"
> > +	"qb save [interface] [dev]  - save quick boot data in NVM location    => trigger quick boot flow\n"
> > +	"qb erase [interface] [dev] - erase quick boot data from NVM location => trigger training flow\n"
> > +);
> > diff --git a/arch/arm/mach-imx/imx9/Makefile b/arch/arm/mach-imx/imx9/Makefile
> > index 3018d128a36..7dee144e0f4 100644
> > --- a/arch/arm/mach-imx/imx9/Makefile
> > +++ b/arch/arm/mach-imx/imx9/Makefile
> > @@ -15,5 +15,7 @@ obj-y += imx_bootaux.o
> >   endif
> >   ifeq ($(CONFIG_IMX_SNPS_DDR_PHY_QB_GEN),y)
> > -obj-y += qb.o
> > +ifneq ($(CONFIG_XPL_BUILD),y)
> > +obj-$(CONFIG_CMD_IMX_QB) += qb.o
> > +endif
> 
> This shoudld be part of 1/5 , should it not ?

And squash commits 1 and 2, as this is cmd_qb related?
Can't I keep them separated, so there is not a lot of code
in one patch? I also find that this way I can explain each
piece of code in the commit message.