From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bin Meng <bmeng.cn@gmail.com>
Date: Mon, 17 Sep 2018 13:02:22 +0800
Subject: [U-Boot] [RESEND PATCH v2 15/15] riscv: Move do_reset() to a
	common place
In-Reply-To: <5c09d2a3b6e3214239633dc6e1a01a65c53c16f3.camel@aisec.fraunhofer.de>
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>
Message-ID: <CAEUhbmVsTsRHW+bS9FG7AbCWYpaMzAcm-CRUuT90m2fxUZaLAg@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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.

> - do_reset() only prints "resetting..." and then calls cpu_reset()
> - reset.c includes a weak cpu_reset() function that prints the warning
> you currently have in this patch (by the way, it should say "reset not
> supported yet"). It might also make sense to print the warning with the
> panic() function and define PANIC_HANG, so that u-boot is halted on
> resets. cpu_reset() can then be defined by boards that have an actual
> reset method.

Regards,
Bin