Hi Heinrich, On Thu, 20 Dec 2018 at 18:58, Heinrich Schuchardt <[email protected]> wrote: > > On 12/20/18 10:17 PM, Simon Glass wrote: > > Hi Heinrich, > > > > On Thu, 20 Dec 2018 at 05:23, Heinrich Schuchardt <[email protected]> > > wrote: > >> > >> On 11/27/18 1:08 AM, Simon Glass wrote: > >>> Hi Heinrich, > >>> > >>> On Sat, 17 Nov 2018 at 07:29, Heinrich Schuchardt <[email protected]> > >>> 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 <[email protected]> > >>>> --- > >>>> 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 <[email protected]> > >>>> + */ > >>>> +#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 > > > _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
