From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Sat, 6 Mar 2021 08:18:10 +0100 Subject: [PATCH 3/5] sysreset: provide SBI based sysreset driver In-Reply-To: References: <20210304170051.58993-1-xypron.glpk@gmx.de> <20210304170051.58993-4-xypron.glpk@gmx.de> Message-ID: <3c1a7db4-204a-d395-337f-b51647e6562b@gmx.de> 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/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. I will change this to a warning. > >> +??? 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". > >> +??? { } >> +}; >> + >> +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 >> >