| Message ID | 20210304170051.58993-4-xypron.glpk@gmx.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Andes |
| Headers | show |
| Series | riscv: enable SBI system reset | expand |
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 >
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 <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. 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 >> >
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 <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. > > 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 >>> >> >
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; + + 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) +{ + 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; + } + + sbi_srst_reset(reset_type, SBI_SRST_RESET_REASON_NONE); + +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"); + return -ENOENT; +} + +static const struct udevice_id sbi_sysreset_ids[] = { + { .compatible = "riscv" }, + { } +}; + +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 config EFI_GRUB_ARM32_WORKAROUND bool "Workaround for GRUB on 32bit ARM"
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 -- 2.30.1