From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Anderson Date: Sat, 6 Mar 2021 11:40:44 -0500 Subject: [PATCH 3/5] sysreset: provide SBI based sysreset driver In-Reply-To: <3c1a7db4-204a-d395-337f-b51647e6562b@gmx.de> References: <20210304170051.58993-1-xypron.glpk@gmx.de> <20210304170051.58993-4-xypron.glpk@gmx.de> <3c1a7db4-204a-d395-337f-b51647e6562b@gmx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 3/6/21 2:18 AM, Heinrich Schuchardt wrote: > On 3/5/21 12:50 AM, Sean Anderson wrote: >> On 3/4/21 12:00 PM, Heinrich Schuchardt wrote: >>> Provide sysreset driver using the SBI system reset extension. >>> >>> Signed-off-by: Heinrich Schuchardt >>> --- >>> 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 >>> +#include >>> #include >>> #include >>> >>> -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 >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +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. > > Thank you for reviewing. > > Yes, we can skip it. > >> >>> + >>> + 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)? > > efi_reset_system is invoked at UEFI runtime. Most functions of U-Boot > are no longer in memory after ExitBootServices(). Only functions in the > __efi_runtime section may be called. > >> >>> +{ >>> + 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; > > ok > >> >>> + } >>> + >>> + 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? > > Makes sense. > >> >>> + >>> +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. > > We shouldn't have chosen this driver in the configuration if the SBI > does not support it. Yes, but we would like to use the same config for differing SBI implementations (e.g. say a vendor's SBI implementation and SBI). > I will change this to a warning. Great. > >> >>> + 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? > > I discussed this with Simon in > > https://lists.denx.de/pipermail/u-boot/2021-March/443270.html > > Instead of adding code to the board files we should have a device-tree > node. A reasonable compatible string would be "riscv,sbi-sysreset". Ok, so will patch 5 have the board parts dropped? --Sean >> >>> + { } >>> +}; >>> + >>> +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? > > This seems to be a matter of taste. > > Best regards > > Heinrich > >> >> --Sean >> >>> >>> config EFI_GRUB_ARM32_WORKAROUND >>> bool "Workaround for GRUB on 32bit ARM" >>> -- >>> 2.30.1 >>> >> >