Hi Heinrich, On Mon, Sep 6, 2021 at 12:50 AM Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > On 9/5/21 1:59 PM, Bin Meng wrote: > > On Sun, Sep 5, 2021 at 4:38 PM Heinrich Schuchardt <xypron.g...@gmx.de> > > wrote: > >> > >> Provide sysreset driver using the SBI system reset extension. > >> > > > > This patch should be split into 2 patches, one for adding the sysreset > > DM driver, and the other one for EFI support. > > > >> Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de> > >> --- > >> v3: > >> no change > >> --- > >> MAINTAINERS | 1 + > >> arch/riscv/cpu/cpu.c | 13 ++++- > >> 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 | 96 +++++++++++++++++++++++++++++++++ > >> lib/efi_loader/Kconfig | 2 +- > >> 8 files changed, 140 insertions(+), 6 deletions(-) > >> create mode 100644 drivers/sysreset/sysreset_sbi.c > >> > >> diff --git a/MAINTAINERS b/MAINTAINERS > >> index 4cf0c33c5d..88d7aa2bc7 100644 > >> --- a/MAINTAINERS > >> +++ b/MAINTAINERS > >> @@ -1017,6 +1017,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/cpu/cpu.c b/arch/riscv/cpu/cpu.c > >> index c894ac10b5..8e49b6d736 100644 > >> --- a/arch/riscv/cpu/cpu.c > >> +++ b/arch/riscv/cpu/cpu.c > >> @@ -6,6 +6,7 @@ > >> #include <common.h> > >> #include <cpu.h> > >> #include <dm.h> > >> +#include <dm/lists.h> > >> #include <init.h> > >> #include <log.h> > >> #include <asm/encoding.h> > >> @@ -138,7 +139,17 @@ int arch_cpu_init_dm(void) > >> > >> int arch_early_init_r(void) > >> { > >> - return riscv_cpu_probe(); > >> + int ret; > >> + > >> + ret = riscv_cpu_probe(); > >> + if (ret) > >> + return ret; > >> + > >> + if (IS_ENABLED(CONFIG_SYSRESET_SBI)) > >> + device_bind_driver(gd->dm_root, "sbi-sysreset", > >> + "sbi-sysreset", NULL); > >> + > >> + return 0; > >> } > >> > >> /** > >> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h > >> index e9caa78d17..69cddda245 100644 > >> --- a/arch/riscv/include/asm/sbi.h > >> +++ b/arch/riscv/include/asm/sbi.h > >> @@ -154,5 +154,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 ac77ffbc8b..6782331181 100644 > >> --- a/drivers/sysreset/Kconfig > >> +++ b/drivers/sysreset/Kconfig > >> @@ -85,6 +85,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 implementation 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..fec5a66515 > >> --- /dev/null > >> +++ b/drivers/sysreset/sysreset_sbi.c > >> @@ -0,0 +1,96 @@ > >> +// SPDX-License-Identifier: GPL-2.0+ > >> +/* > >> + * Copyright 2021, Heinrich Schuchardt <xypron.g...@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; > >> + > >> + 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; > >> +} > > > > Is there a better place for the EFI stuff? > > Bin, thanks for reviewing the series. > > This seems to be related to you comment above about splitting into two > patches.
Yep. > We put have the same set of EFI runtime functions for system > reset in: > > drivers/sysreset/sysreset_x86.c > drivers/firmware/psci.c > arch/arm/mach-bcm283x/reset.c > arch/arm/cpu/armv8/fsl-layerscape/cpu.c I didn't realize that. But this does not look correct to me. EFI reset should be independent of the system reset drivers. For example, on RISC-V SRST is optional so not every SBI implements this. On SiFive Unleashed, the "gpio-restart" driver is used for reset (see sysreset_gpio.c). With the approach in this patch, we mandate the EFI reset goes through the SBI call hence it does not work on boards like Unleashed. I believe we should drop the driver-specific efi_reset_system(), but move efi_reset_system() to a generic place in EFI loader, and re-implement it using DM APIs. > > My idea was that all SBI system reset related things should be in the > same source file. > > Where do you think would be the best place for RISC-V SBI calls at UEFI > runtime? > Regards, Bin
