[PATCH 3/5] sysreset: provide SBI based sysreset driver
Heinrich Schuchardt
xypron.glpk at gmx.de
Sat Mar 6 08:18:10 CET 2021
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 at 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 at 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
>>
>
More information about the U-Boot
mailing list