[U-Boot] [PATCH 1/1] cmd: add exception command

[Simon Glass]

Hi Heinrich,

On Thu, 20 Dec 2018 at 18:58, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 12/20/18 10:17 PM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Thu, 20 Dec 2018 at 05:23, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >> On 11/27/18 1:08 AM, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Sat, 17 Nov 2018 at 07:29, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>>>
> >>>> The 'exception' command allows to test exception handling.
> >>>>
> >>>> This implementation supports ARM, x86, RISC-V and the following exceptions:
> >>>> * 'breakpoint' - prefetch abort exception (ARM 32bit only)
> >>>> * 'unaligned'  - data abort exception (ARM only)
> >>>> * 'undefined'  - undefined instruction exception
> >>>>
> >>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> >>>> ---
> >>>> Currently RISC-V 64bit gets into an endless loop when hitting the
> >>>> undefined instruction.
> >>>>
> >>>> So it makes sense to have a testing capability.
> >>>> ---
> >>>>  cmd/Kconfig     |   6 ++
> >>>>  cmd/Makefile    |   1 +
> >>>>  cmd/exception.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 168 insertions(+)
> >>>>  create mode 100644 cmd/exception.c
> >>>>
> >>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
> >>>> index e2973b3c51..9d2b8199b8 100644
> >>>> --- a/cmd/Kconfig
> >>>> +++ b/cmd/Kconfig
> >>>> @@ -1388,6 +1388,12 @@ config CMD_DISPLAY
> >>>>           displayed on a simple board-specific display. Implement
> >>>>           display_putc() to use it.
> >>>>
> >>>> +config CMD_EXCEPTION
> >>>> +       bool "exception - raise exception"
> >>>> +       depends on ARM || RISCV || X86
> >>>> +       help
> >>>> +         Enable the 'exception' command which allows to raise an exception.
> >>>> +
> >>>>  config CMD_LED
> >>>>         bool "led"
> >>>>         default y if LED
> >>>> diff --git a/cmd/Makefile b/cmd/Makefile
> >>>> index 0534ddc679..cd67a79170 100644
> >>>> --- a/cmd/Makefile
> >>>> +++ b/cmd/Makefile
> >>>> @@ -46,6 +46,7 @@ endif
> >>>>  obj-$(CONFIG_CMD_DISPLAY) += display.o
> >>>>  obj-$(CONFIG_CMD_DTIMG) += dtimg.o
> >>>>  obj-$(CONFIG_CMD_ECHO) += echo.o
> >>>> +obj-$(CONFIG_CMD_EXCEPTION) += exception.o
> >>>>  obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o
> >>>>  obj-$(CONFIG_CMD_EEPROM) += eeprom.o
> >>>>  obj-$(CONFIG_EFI_STUB) += efi.o
> >>>> diff --git a/cmd/exception.c b/cmd/exception.c
> >>>> new file mode 100644
> >>>> index 0000000000..fb6a15573e
> >>>> --- /dev/null
> >>>> +++ b/cmd/exception.c
> >>>> @@ -0,0 +1,161 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0+
> >>>> +/*
> >>>> + * The 'exception' command can be used for testing exception handling.
> >>>> + *
> >>>> + * Copyright (c) 2018, Heinrich Schuchardt <xypron.glpk at gmx.de>
> >>>> + */
> >>>> +#include <common.h>
> >>>> +#include <command.h>
> >>>> +
> >>>> +enum exception {
> >>>> +       UNDEFINED_INSTRUCTION = 1,
> >>>> +       DATA_ABORT,
> >>>> +       BREAKPOINT,
> >>>> +};
> >>>> +
> >>>> +struct exception_str {
> >>>> +       enum exception id;
> >>>> +       char *text;
> >>>> +       void (*func)(void);
> >>>> +};
> >>>
> >>> Can you use the normal subcommand approach for this, as with other commands?
> >>
> >> Could you give an example, please. I looked at cmd/scsi. and cmd/mmc.c
> >> And the only difference I spot is that the names of subcommand functions
> >> start with "do_".
> >
> > Yes, see for example:
> >
> > static cmd_tbl_t cmd_mmc[] = {
> >
> > It defines the subcommands in a standard way.
>
> Hello Simon,
>
> thanks for pointing me to the right place. I will adjust my patch.
>
> I think we have some design issues with sub-commands:
>
> I want to have auto-completion for the sub-commands. Unfortunately the
> "standard way" does not offer it out of the box.
>
> The standard way further forces the creation of redundant code like:
>
> static int do_zynq(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> argv[])
> {
>         cmd_tbl_t *zynq_cmd;
>         int ret;
>
>         if (!ARRAY_SIZE(zynq_commands)) {
>                 puts("No zynq specific command enabled\n");
>                 return CMD_RET_USAGE;
>         }
>
>         if (argc < 2)
>                 return CMD_RET_USAGE;
>         zynq_cmd = find_cmd_tbl(argv[1], zynq_commands,
>                                 ARRAY_SIZE(zynq_commands));
>         if (!zynq_cmd || argc != zynq_cmd->maxargs)
>                 return CMD_RET_USAGE;
>
>         ret = zynq_cmd->cmd(zynq_cmd, flag, argc, argv);
>
>         return cmd_process_error(zynq_cmd, ret);
> }
>
> It would be preferable to reference cmd_tbl_t in U_BOOT_CMD. Furthermore
> U_BOOT_CMD_MKENT should allow references to a cmd_tbl_t for sub-sub
> commands. This way autocompletion of subcommands could be provided.
>
> This would also allow to place the help texts for subcommands into
> U_BOOT_CMD_MKENT where they belong.

Yes indeed. There was a recent series sent to introduce
auto-completion for that I think. I have not looked at it though.

>
> Why are U_BOOT_SUBCMD_START and U_BOOT_SUBCMD_END only defined for
> CONFIG_CMDLINE=n? This way we cannot use them.

Well it was a feature designed to provide a way to drop commands. I
have no objection to changing this.

>
> Why do we check CONFIG_SYS_LONGHELP inside cmd/*.c for many commands?
> Hasn't this been superseded by _CMD_HELP in include/commands.h?

Looks like it. Yes this could be cleaned up.

Regards,
Simon


>
> Best regards
>
> Heinrich
>
> >
> >>
> >>>
> >>>> +
> >>>> +#if defined(CONFIG_ARM)
> >>>
> >>> How about creating a uclass for this? It isn't nice to have a
> >>> arch-specific #ifdefs in commands. Something like we did with
> >>> sysreset.
> >>
> >> If you want separate files per architecture, why would we need a uclass?
> >>
> >> We could simply put the architecture specific U_BOOT_CMD into the
> >> implementation file, e.g arch/arm/cpu/exception.c and
> >> arch/arm/cpu/exception_64.c
> >
> > That that is one option although perhaps you might end up with
> > multiple ARM implementation (e.g. for ARMv47, ARMv7 and ARMv8)?
> >
> >>
> >> With a uclass I would not know how to implement an architecture specific
> >> help output.
> >
> > One option is something like sysreset_walk(). It allows us to have a
> > common function for ARM undefined instruction, for example, but finer
> > granularity for breakpoint.
> >
> > Regards,
> > Simon
> >
>