[PATCH 3/5] sysreset: provide SBI based sysreset driver

[Sean Anderson <seanga2@gmail.com>]

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
>>>
>>
>