Hi Lukas,
On Tue, Sep 18, 2018 at 6:01 AM Auer, Lukas lukas.auer@aisec.fraunhofer.de wrote:
Hi Bin,
On Mon, 2018-09-17 at 13:02 +0800, Bin Meng wrote:
Hi Lukas,
On Mon, Sep 17, 2018 at 5:09 AM Auer, Lukas lukas.auer@aisec.fraunhofer.de wrote:
Hi Bin,
On Mon, 2018-09-10 at 21:54 -0700, Bin Meng wrote:
We don't have a reset method on any RISC-V board yet. Instead of adding the same 'unsupported' message for each CPU variant it might make more sense to add a generic do_reset function for all CPU variants to lib/, similar to the one for ARM (arch/arm/lib/reset.c).
Suggested-by: Lukas Auer lukas.auer@aisec.fraunhofer.de Signed-off-by: Bin Meng bmeng.cn@gmail.com
Changes in v2:
- new patch to move do_reset() to a common place
arch/riscv/cpu/ax25/cpu.c | 9 --------- arch/riscv/cpu/qemu/cpu.c | 8 -------- arch/riscv/lib/Makefile | 1 + arch/riscv/lib/reset.c | 14 ++++++++++++++ 4 files changed, 15 insertions(+), 17 deletions(-) create mode 100644 arch/riscv/lib/reset.c
diff --git a/arch/riscv/cpu/ax25/cpu.c b/arch/riscv/cpu/ax25/cpu.c index ab05b57..fddcc15 100644 --- a/arch/riscv/cpu/ax25/cpu.c +++ b/arch/riscv/cpu/ax25/cpu.c @@ -6,9 +6,6 @@
/* CPU specific code */ #include <common.h> -#include <command.h> -#include <watchdog.h> -#include <asm/cache.h>
/*
- cleanup_before_linux() is called just before we call linux
@@ -24,9 +21,3 @@ int cleanup_before_linux(void)
return 0;}
-int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) -{
disable_interrupts();panic("ax25-ae350 wdt not support yet.\n");-} diff --git a/arch/riscv/cpu/qemu/cpu.c b/arch/riscv/cpu/qemu/cpu.c index a064639..6c7a327 100644 --- a/arch/riscv/cpu/qemu/cpu.c +++ b/arch/riscv/cpu/qemu/cpu.c @@ -4,7 +4,6 @@ */
#include <common.h> -#include <command.h>
/*
- cleanup_before_linux() is called just before we call linux
@@ -20,10 +19,3 @@ int cleanup_before_linux(void)
return 0;}
-int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) -{
printf("reset unsupported yet\n");return 0;-} diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index cc562f9..b58db89 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o obj-$(CONFIG_CMD_GO) += boot.o obj-y += cache.o obj-y += interrupts.o +obj-y += reset.o obj-y += setjmp.o
# For building EFI apps diff --git a/arch/riscv/lib/reset.c b/arch/riscv/lib/reset.c new file mode 100644 index 0000000..5d9b99c --- /dev/null +++ b/arch/riscv/lib/reset.c @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (C) 2018, Bin Meng bmeng.cn@gmail.com
- */
+#include <common.h> +#include <command.h>
+int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{
printf("reset unsupported yet\n");return 0;+}
Thanks for adding this patch! I see one possible problem with it. The way you implemented it, arch / board code can't overwrite the function to add a reset method. How about something like this?
Thanks for your review and comments. I believe current implementation in this patch is an interim approach, for now to save some duplicates. The problem you mentioned can be resolved by implementing a SYSRESET uclass driver for that CPU/board in the future.
That's true, still, two minor things I would consider to change.
- A simple wording changes to make it clear that the reset is not only
not available, but u-boot actually tried to reset the system. So something like this: "Resetting... reset not supported yet".
Sure, will reword this in v3.
- I don't know how much of an issue it is that u-boot keeps running
after a reset. I would tend to use panic() together with PANIC_HANG, to make sure u-boot halts. You know u-boot better than me, what do you think?
Yes, I think using hang() can be a good choice for now.
Regards, Bin