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

[Heinrich Schuchardt]

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.

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

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?

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
>