Re: [PATCH 3/5] sysreset: provide SBI based sysreset driver

On 3/4/21 12:00 PM, Heinrich Schuchardt wrote:

Provide sysreset driver using the SBI system reset extension.

Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de

MAINTAINERS | 1 + arch/riscv/include/asm/sbi.h | 1 + arch/riscv/lib/sbi.c | 21 +++++-- drivers/sysreset/Kconfig | 11 ++++ drivers/sysreset/Makefile | 1 + drivers/sysreset/sysreset_sbi.c | 102 ++++++++++++++++++++++++++++++++ lib/efi_loader/Kconfig | 2 +- 7 files changed, 134 insertions(+), 5 deletions(-) create mode 100644 drivers/sysreset/sysreset_sbi.c

diff --git a/MAINTAINERS b/MAINTAINERS index de499940e5..192db06ff9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -985,6 +985,7 @@ T: git https://source.denx.de/u-boot/custodians/u-boot-riscv.git F: arch/riscv/ F: cmd/riscv/ F: doc/usage/sbi.rst +F: drivers/sysreset/sysreset_sbi.c F: drivers/timer/andes_plmt_timer.c F: drivers/timer/sifive_clint_timer.c F: tools/prelink-riscv.c diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h index c598bb11ce..058e2e23a8 100644 --- a/arch/riscv/include/asm/sbi.h +++ b/arch/riscv/include/asm/sbi.h @@ -150,5 +150,6 @@ void sbi_set_timer(uint64_t stime_value); long sbi_get_spec_version(void); int sbi_get_impl_id(void); int sbi_probe_extension(int ext); +void sbi_srst_reset(unsigned long type, unsigned long reason);

#endif diff --git a/arch/riscv/lib/sbi.c b/arch/riscv/lib/sbi.c index 77845a73ca..8508041f2a 100644 --- a/arch/riscv/lib/sbi.c +++ b/arch/riscv/lib/sbi.c @@ -8,13 +8,14 @@ */

#include <common.h> +#include <efi_loader.h> #include <asm/encoding.h> #include <asm/sbi.h>

-struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,

• 	unsigned long arg1, unsigned long arg2,
  

• 	unsigned long arg3, unsigned long arg4,
  

• 	unsigned long arg5)
  

+struct sbiret __efi_runtime sbi_ecall(int ext, int fid, unsigned long arg0,

• 		      unsigned long arg1, unsigned long arg2,
  

• 		      unsigned long arg3, unsigned long arg4,
  

• 		      unsigned long arg5)
  

  { struct sbiret ret;

@@ -108,6 +109,18 @@ int sbi_probe_extension(int extid) return -ENOTSUPP; }

+/**

• • sbi_srst_reset() - invoke system reset extension
• • @type: type of reset
• • @reason: reason for reset
• */

+void __efi_runtime sbi_srst_reset(unsigned long type, unsigned long reason) +{

• sbi_ecall(SBI_EXT_SRST, SBI_EXT_SRST_RESET, type, reason,

•   0, 0, 0, 0);
  

+}

• #ifdef CONFIG_SBI_V01

  /**

diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig index 0e5c7c9971..05a7e26300 100644 --- a/drivers/sysreset/Kconfig +++ b/drivers/sysreset/Kconfig @@ -79,6 +79,17 @@ config SYSRESET_PSCI Enable PSCI SYSTEM_RESET function call. To use this, PSCI firmware must be running on your system.

+config SYSRESET_SBI

• bool "Enable support for SBI System Reset"
• depends on RISCV_SMODE && SBI_V02
• select SYSRESET_CMD_POWEROFF if CMD_POWEROFF
• help

•  Enable system reset and poweroff via the SBI system reset extension.
  

•  If the SBI implemention provides the extension, is board specific.
  

•  The extension was introduced in version 0.3 of the SBI specification.
  

•  The SBI system reset driver supports the UEFI ResetSystem() service
  

•  at runtime.
  

• config SYSRESET_SOCFPGA bool "Enable support for Intel SOCFPGA family" depends on ARCH_SOCFPGA && (TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10)

diff --git a/drivers/sysreset/Makefile b/drivers/sysreset/Makefile index de81c399d7..8e00be0779 100644 --- a/drivers/sysreset/Makefile +++ b/drivers/sysreset/Makefile @@ -13,6 +13,7 @@ obj-$(CONFIG_SYSRESET_MPC83XX) += sysreset_mpc83xx.o obj-$(CONFIG_SYSRESET_MICROBLAZE) += sysreset_microblaze.o obj-$(CONFIG_SYSRESET_OCTEON) += sysreset_octeon.o obj-$(CONFIG_SYSRESET_PSCI) += sysreset_psci.o +obj-$(CONFIG_SYSRESET_SBI) += sysreset_sbi.o obj-$(CONFIG_SYSRESET_SOCFPGA) += sysreset_socfpga.o obj-$(CONFIG_SYSRESET_SOCFPGA_SOC64) += sysreset_socfpga_soc64.o obj-$(CONFIG_SYSRESET_TI_SCI) += sysreset-ti-sci.o diff --git a/drivers/sysreset/sysreset_sbi.c b/drivers/sysreset/sysreset_sbi.c new file mode 100644 index 0000000000..87fbc3ea3e --- /dev/null +++ b/drivers/sysreset/sysreset_sbi.c @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright 2021, Heinrich Schuchardt xypron.glpk@gmx.de
• */

+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <efi_loader.h> +#include <log.h> +#include <sysreset.h> +#include <asm/sbi.h>

+static long __efi_runtime_data have_reset;

+static int sbi_sysreset_request(struct udevice *dev, enum sysreset_t type) +{

• enum sbi_srst_reset_type reset_type;
• if (!have_reset)

• return -ENOENT;
  

Is this necessary? This should never be called if there is no reset, since we just fail to probe.

• switch (type) {
• case SYSRESET_WARM:

• reset_type = SBI_SRST_RESET_TYPE_WARM_REBOOT;
  

• break;
  

• case SYSRESET_COLD:

• reset_type = SBI_SRST_RESET_TYPE_COLD_REBOOT;
  

• break;
  

• case SYSRESET_POWER_OFF:

• reset_type = SBI_SRST_RESET_TYPE_SHUTDOWN;
  

• break;
  

• default:

• log_err("SBI has no system reset extension\n");
  

• return -ENOSYS;
  

• }
• sbi_srst_reset(reset_type, SBI_SRST_RESET_REASON_NONE);
• return -EINPROGRESS;

+}

+efi_status_t efi_reset_system_init(void) +{

• return EFI_SUCCESS;

+}

+void __efi_runtime EFIAPI efi_reset_system(enum efi_reset_type type,

• 			   efi_status_t reset_status,
  

• 			   unsigned long data_size,
  

• 			   void *reset_data)
  

As an aside, is there a reason why the generic (weak) efi_reset_system does not just call sysreset_walk_halt(type)?

+{

• enum sbi_srst_reset_type reset_type;
• if (have_reset)

• switch (type) {
  

• case SYSRESET_WARM:
  

• 	reset_type = SBI_SRST_RESET_TYPE_WARM_REBOOT;
  

• 	break;
  

• case SYSRESET_COLD:
  

• 	reset_type = SBI_SRST_RESET_TYPE_COLD_REBOOT;
  

• 	break;
  

• case SYSRESET_POWER_OFF:
  

• 	reset_type = SBI_SRST_RESET_TYPE_SHUTDOWN;
  

• 	break;
  

• default:
  

• 	goto hang;
  

Why do we hang by default? For comparison, sysreset_x86 has

if (reset_type == EFI_RESET_COLD || reset_type == EFI_RESET_PLATFORM_SPECIFIC) value = SYS_RST | RST_CPU | FULL_RST; else /* assume EFI_RESET_WARM since we cannot return an error */ value = SYS_RST | RST_CPU;

• }
• sbi_srst_reset(reset_type, SBI_SRST_RESET_REASON_NONE);

Should we instead do

enum sbi_srst_reset_reason reset_reason; if (reset_status == EFI_SUCCESS) reset_reason = SBI_SRST_RESET_REASON_NONE; else reset_reason = SBI_SRST_RESET_REASON_SYS_FAILURE; sbi_srst_reset(reset_type, reset_status == EFI_SUCCESS ? SBI_SRST_RESET_REASON_NONE : SBI_SRST_RESET_REASON_SYS_FAILURE);

since we have reset status available?

+hang:

• while (1)

• ;
  

+}

+static int sbi_sysreset_probe(struct udevice *dev) +{

• have_reset = sbi_probe_extension(SBI_EXT_SRST);
• if (have_reset)

• return 0;
  

• log_err("SBI has no system reset extension\n");

Is this an error? It's perfectly normal for SBI implementations to lack support for some extensions. Perhaps a warning/info would be better.

• return -ENOENT;

+}

+static const struct udevice_id sbi_sysreset_ids[] = {

• { .compatible = "riscv" },

This compatible string is already in-use for RISC-V CPUs. And if this isn't supposed to have a device tree binding, do we even need of_match?

• { }

+};

+static struct sysreset_ops sbi_sysreset_ops = {

• .request = sbi_sysreset_request,

+};

+U_BOOT_DRIVER(sbi_sysreset) = {

• .name = "sbi-sysreset",
• .id = UCLASS_SYSRESET,
• .of_match = sbi_sysreset_ids,
• .ops = &sbi_sysreset_ops,
• .probe = sbi_sysreset_probe,

+}; diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index e729f727df..7e76435339 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -277,7 +277,7 @@ config EFI_HAVE_RUNTIME_RESET bool default y depends on ARCH_BCM283X || FSL_LAYERSCAPE || PSCI_RESET || \

•    SANDBOX || SYSRESET_X86
  

•    SANDBOX || SYSRESET_SBI || SYSRESET_X86
  

As a general note, perhaps it is better to set this from other Kconfigs? How long will this list get?

--Sean

config EFI_GRUB_ARM32_WORKAROUND bool "Workaround for GRUB on 32bit ARM" -- 2.30.1