[PATCH v2 07/16] x86: zboot: Set up a sub-command structure

[Simon Glass]

Hi Bin,

On Tue, 1 Sep 2020 at 03:30, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Sun, Aug 30, 2020 at 5:42 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > Add subcommands to zboot. At present there is only one called 'start'
> > which does the whole boot. It is the default command so is optional.
> >
> > Change the 's' string variable to const while we are here.
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v2:
> > - Fix comment about argv[0] in do_zboot_parent()
> >
> >  arch/x86/lib/zimage.c | 64 +++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 58 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
> > index 8651dea93b3..e3e3efdde43 100644
> > --- a/arch/x86/lib/zimage.c
> > +++ b/arch/x86/lib/zimage.c
> > @@ -63,6 +63,12 @@ struct zboot_state {
> >         ulong load_address;
> >  } state;
> >
> > +enum {
> > +       ZBOOT_STATE_START       = BIT(0),
> > +
> > +       ZBOOT_STATE_COUNT       = 1,
> > +};
> > +
> >  static void build_command_line(char *command_line, int auto_boot)
> >  {
> >         char *env_command_line;
> > @@ -328,10 +334,11 @@ int setup_zimage(struct boot_params *setup_base, char *cmd_line, int auto_boot,
> >         return 0;
> >  }
> >
> > -int do_zboot(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> > +static int do_zboot_start(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                         char *const argv[])
> >  {
> >         struct boot_params *base_ptr;
> > -       char *s;
> > +       const char *s;
> >
> >         memset(&state, '\0', sizeof(state));
> >         if (argc >= 2) {
> > @@ -373,9 +380,53 @@ int do_zboot(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >         return boot_linux_kernel((ulong)base_ptr, state.load_address, false);
> >  }
> >
> > -U_BOOT_CMD(
> > -       zboot, 5, 0,    do_zboot,
> > -       "Boot bzImage",
> > +U_BOOT_SUBCMDS(zboot,
> > +       U_BOOT_CMD_MKENT(start, 8, 1, do_zboot_start, "", ""),
>
> should the maxargs be 5 instead of 8?

Yes, for this patch. I'll update it and adjust it in each following
patch as needed.

>
> > +)
> > +
> > +int do_zboot_states(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                   char *const argv[], int state_mask)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ZBOOT_STATE_COUNT; i++) {
> > +               struct cmd_tbl *cmd = &zboot_subcmds[i];
> > +               int mask = 1 << i;
> > +               int ret;
> > +
> > +               if (mask & state_mask) {
> > +                       ret = cmd->cmd(cmd, flag, argc, argv);
> > +                       if (ret)
> > +                               return ret;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +int do_zboot_parent(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                   char *const argv[], int *repeatable)
> > +{
> > +       /* determine if we have a sub command */
> > +       if (argc > 1) {
> > +               char *endp;
> > +
> > +               simple_strtoul(argv[1], &endp, 16);
> > +               /*
> > +                * endp pointing to nul means that argv[1] was just a valid
> > +                * number, so pass it along to the normal processing
> > +                */
> > +               if (*endp)
> > +                       return do_zboot(cmdtp, flag, argc, argv, repeatable);
> > +       }
> > +
> > +       do_zboot_states(cmdtp, flag, argc, argv, ZBOOT_STATE_START);
> > +
> > +       return CMD_RET_FAILURE;
> > +}
> > +
> > +U_BOOT_CMDREP_COMPLETE(
> > +       zboot, 8, do_zboot_parent, "Boot bzImage",
> >         "[addr] [size] [initrd addr] [initrd size]\n"
> >         "      addr -        The optional starting address of the bzimage.\n"
> >         "                    If not set it defaults to the environment\n"
> > @@ -383,5 +434,6 @@ U_BOOT_CMD(
> >         "      size -        The optional size of the bzimage. Defaults to\n"
> >         "                    zero.\n"
> >         "      initrd addr - The address of the initrd image to use, if any.\n"
> > -       "      initrd size - The size of the initrd image to use, if any.\n"
> > +       "      initrd size - The size of the initrd image to use, if any.\n",
> > +       complete_zboot
>
> What's "complete_zboot"?

It is defined by the U_BOOT_SUBCMDS() macro. I'll add a comment.

Regards,
SImon