From mboxrd@z Thu Jan 1 00:00:00 1970 From: Auer, Lukas Date: Tue, 18 Sep 2018 10:54:49 +0000 Subject: [U-Boot] [RESEND PATCH v2 15/15] riscv: Move do_reset() to a common place In-Reply-To: References: <1536641694-4200-1-git-send-email-bmeng.cn@gmail.com> <1536641694-4200-16-git-send-email-bmeng.cn@gmail.com> <5c09d2a3b6e3214239633dc6e1a01a65c53c16f3.camel@aisec.fraunhofer.de> <02a5863da86cfcbf2c75aac5af0984c35cb739b7.camel@aisec.fraunhofer.de> Message-ID: <7624ca568eddee67107f2113feaee09803b59cff.camel@aisec.fraunhofer.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Bin, On Tue, 2018-09-18 at 16:50 +0800, Bin Meng wrote: > Hi Lukas, > > On Tue, Sep 18, 2018 at 6:01 AM Auer, Lukas > 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 > > > 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 > > > > > Signed-off-by: Bin Meng > > > > > > > > > > --- > > > > > > > > > > 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 > > > > > -#include > > > > > -#include > > > > > -#include > > > > > > > > > > /* > > > > > * 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 > > > > > -#include > > > > > > > > > > /* > > > > > * 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 > > > > > + */ > > > > > + > > > > > +#include > > > > > +#include > > > > > + > > > > > +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 Thanks! :)