[U-Boot] [RESEND PATCH v2 15/15] riscv: Move do_reset() to a common place

[Auer, Lukas]

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
> <lukas.auer at 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 at 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 at aisec.fraunhofer.de>
> > > > > Signed-off-by: Bin Meng <bmeng.cn at 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 at 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

Thanks! :)