[RFC PATCH v2] lib: Implement System Reset (SRST) SBI extension
Anup Patel
Anup.Patel at wdc.com
Sat Oct 3 10:49:17 EDT 2020
> -----Original Message-----
> From: Sean Anderson <seanga2 at gmail.com>
> Sent: 02 October 2020 21:05
> To: Anup Patel <Anup.Patel at wdc.com>; Atish Patra
> <Atish.Patra at wdc.com>; Alistair Francis <Alistair.Francis at wdc.com>
> Cc: Anup Patel <anup at brainfault.org>; opensbi at lists.infradead.org
> Subject: Re: [RFC PATCH v2] lib: Implement System Reset (SRST) SBI
> extension
>
> On 10/1/20 3:56 AM, Anup Patel wrote:
> >
> >
> >> -----Original Message-----
> >> From: Sean Anderson <seanga2 at gmail.com>
> >> Sent: 30 September 2020 17:07
> >> To: Anup Patel <Anup.Patel at wdc.com>; Atish Patra
> >> <Atish.Patra at wdc.com>; Alistair Francis <Alistair.Francis at wdc.com>
> >> Cc: Anup Patel <anup at brainfault.org>; opensbi at lists.infradead.org
> >> Subject: Re: [RFC PATCH v2] lib: Implement System Reset (SRST) SBI
> >> extension
> >>
> >> On 9/26/20 2:53 AM, Anup Patel wrote:
> >>> The SBI SRST extension specification is in draft state and available
> >>> in srbt_v1 branch of: https://github.com/avpatel/riscv-sbi-doc.
> >>>
> >>> It allows to S-mode software to request system shutdown, cold
> >>> reboot, and warm reboot.
> >>>
> >>> This patch provides implementation of SBI SRST extension.
> >>>
> >>> Signed-off-by: Anup Patel <anup.patel at wdc.com>
> >>> ---
> >>> Changes since v1:
> >>> - Updated patch as-per latest SBI SRST extension draft spec where
> >>> we have only one SBI call with "reset_type" parameter
> >>> ---
> >>> include/sbi/sbi_ecall.h | 1 +
> >>> include/sbi/sbi_ecall_interface.h | 8 ++++++
> >>> lib/sbi/objects.mk | 1 +
> >>> lib/sbi/sbi_ecall.c | 3 ++
> >>> lib/sbi/sbi_ecall_srst.c | 48 +++++++++++++++++++++++++++++++
> >>> 5 files changed, 61 insertions(+)
> >>> create mode 100644 lib/sbi/sbi_ecall_srst.c
> >>>
> >>> diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h index
> >>> 3273ba6..1ef86e2 100644
> >>> --- a/include/sbi/sbi_ecall.h
> >>> +++ b/include/sbi/sbi_ecall.h
> >>> @@ -37,6 +37,7 @@ extern struct sbi_ecall_extension ecall_rfence;
> >>> extern struct sbi_ecall_extension ecall_ipi; extern struct
> >>> sbi_ecall_extension ecall_vendor; extern struct sbi_ecall_extension
> >>> ecall_hsm;
> >>> +extern struct sbi_ecall_extension ecall_srst;
> >>>
> >>> u16 sbi_ecall_version_major(void);
> >>>
> >>> diff --git a/include/sbi/sbi_ecall_interface.h
> >>> b/include/sbi/sbi_ecall_interface.h
> >>> index af30500..d9c2519 100644
> >>> --- a/include/sbi/sbi_ecall_interface.h
> >>> +++ b/include/sbi/sbi_ecall_interface.h
> >>> @@ -27,6 +27,7 @@
> >>> #define SBI_EXT_IPI 0x735049
> >>> #define SBI_EXT_RFENCE 0x52464E43
> >>> #define SBI_EXT_HSM 0x48534D
> >>> +#define SBI_EXT_SRST 0x53525354
> >>>
> >>> /* SBI function IDs for BASE extension*/
> >>> #define SBI_EXT_BASE_GET_SPEC_VERSION 0x0
> >>> @@ -62,6 +63,13 @@
> >>> #define SBI_HSM_HART_STATUS_START_PENDING 0x2
> >>> #define SBI_HSM_HART_STATUS_STOP_PENDING 0x3
> >>>
> >>> +/* SBI function IDs for SRST extension */
> >>> +#define SBI_EXT_SRST_RESET 0x0
> >>> +
> >>> +#define SBI_SRST_RESET_TYPE_SHUTDOWN 0x0
> >>> +#define SBI_SRST_RESET_TYPE_COLD_REBOOT 0x1
> >>> +#define SBI_SRST_RESET_TYPE_WARM_REBOOT 0x2
> >>> +
> >>> #define SBI_SPEC_VERSION_MAJOR_OFFSET 24
> >>> #define SBI_SPEC_VERSION_MAJOR_MASK 0x7f
> >>> #define SBI_SPEC_VERSION_MINOR_MASK 0xffffff
> >>> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk index
> >>> fa808a0..6ec1154 100644
> >>> --- a/lib/sbi/objects.mk
> >>> +++ b/lib/sbi/objects.mk
> >>> @@ -20,6 +20,7 @@ libsbi-objs-y += sbi_ecall_base.o libsbi-objs-y
> >>> += sbi_ecall_hsm.o libsbi-objs-y += sbi_ecall_legacy.o
> >>> libsbi-objs-y += sbi_ecall_replace.o
> >>> +libsbi-objs-y += sbi_ecall_srst.o
> >>> libsbi-objs-y += sbi_ecall_vendor.o libsbi-objs-y +=
> >>> sbi_emulate_csr.o libsbi-objs-y += sbi_fifo.o diff --git
> >>> a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c index 64c9933..6d41cff
> >>> 100644
> >>> --- a/lib/sbi/sbi_ecall.c
> >>> +++ b/lib/sbi/sbi_ecall.c
> >>> @@ -167,6 +167,9 @@ int sbi_ecall_init(void)
> >>> if (ret)
> >>> return ret;
> >>> ret = sbi_ecall_register_extension(&ecall_hsm);
> >>> + if (ret)
> >>> + return ret;
> >>> + ret = sbi_ecall_register_extension(&ecall_srst);
> >>> if (ret)
> >>> return ret;
> >>> ret = sbi_ecall_register_extension(&ecall_legacy);
> >>> diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c new
> >>> file mode 100644 index 0000000..64c228f
> >>> --- /dev/null
> >>> +++ b/lib/sbi/sbi_ecall_srst.c
> >>> @@ -0,0 +1,48 @@
> >>> +/*
> >>> + * SPDX-License-Identifier: BSD-2-Clause
> >>> + *
> >>> + * Copyright (c) 2020 Western Digital Corporation or its affiliates.
> >>> + *
> >>> + * Authors:
> >>> + * Anup Patel <anup.patel at wdc.com>
> >>> + */
> >>> +
> >>> +#include <sbi/sbi_ecall.h>
> >>> +#include <sbi/sbi_ecall_interface.h> #include <sbi/sbi_error.h>
> >>> +#include <sbi/sbi_platform.h> #include <sbi/sbi_system.h>
> >>> +
> >>> +static int sbi_ecall_srst_handler(unsigned long extid, unsigned
> >>> +long
> >> funcid,
> >>> + unsigned long *args, unsigned long
> >> *out_val,
> >>> + struct sbi_trap_info *out_trap) {
> >>> + int ret = 0;
> >>> +
> >>> + if (funcid == SBI_EXT_SRST_RESET) {
> >>> + switch (args[0]) {
> >>> + case SBI_SRST_RESET_TYPE_SHUTDOWN:
> >>> +
> >> sbi_system_reset(SBI_PLATFORM_RESET_SHUTDOWN);
> >>
> >> Shouldn't we check a return value here? Some platforms may not
> >> support all types of resets, and may want to return SBI_ENOTSUPP. For
> >> example, the
> >> K210 only supports warm resets.
> >
> > The sbi_system_reset() is implemented as noreturn function.
> >
> > We have three cases here:
> > 1. A platform has none of the SBI system reset types supported 2. A
> > platform has few of the SBI system reset types supported 3. A platform
> > has all SBI system reset types supported
> >
> > For case#1, we will simply halt all HARTs in WFI.
> >
> > For case#2, we will halt all HARTs in WFI when a particular SBI system
> > reset type is not supported by platform.
> >
> > Basically, current sbi_system_reset() implementation ensures that we
> > never return for a valid reset_type irrespective whether underlying
> > platform supports it or not.
>
> I think that should be changed. There must be a way to discover which reset
> types are supported by the platform. For example, say that I want to reset
> the system, or shut it down only if there is no way to reset it. With this API, I
> would have no way to do this. If I try a cold reset and it is not available, the
> platform will "shut down" anyway, even if there is a warm reset available.
This is a good suggestion. The SBI SRST RESET call should return error when
reset_type is not supported by the platform.
We should first update the SBI SRST extension draft and then update this
patch. (Refer, https://github.com/riscv/riscv-sbi-doc/pull/39)
Regards,
Anup
>
> --Sean
>
> > The only case where SBI SRST RESET call fails is when provided
> > reset_type is invalid.
> >
> > Regards,
> > Anup
> >
> >>
> >> --Sean
> >>
> >>> + break;
> >>> + case SBI_SRST_RESET_TYPE_COLD_REBOOT:
> >>> + sbi_system_reset(SBI_PLATFORM_RESET_COLD);
> >>> + break;
> >>> + case SBI_SRST_RESET_TYPE_WARM_REBOOT:
> >>> + sbi_system_reset(SBI_PLATFORM_RESET_WARM);
> >>> + break;
> >>> + default:
> >>> + ret = SBI_EINVAL;
> >>> + break;
> >>> + };
> >>> + } else {
> >>> + ret = SBI_ENOTSUPP;
> >>> + }
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +struct sbi_ecall_extension ecall_srst = {
> >>> + .extid_start = SBI_EXT_SRST,
> >>> + .extid_end = SBI_EXT_SRST,
> >>> + .handle = sbi_ecall_srst_handler,
> >>> +};
> >>>
> >
More information about the opensbi
mailing list