On Mon, Sep 6, 2021 at 1:21 AM Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
> On 9/5/21 7:00 PM, Bin Meng wrote:
> > 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).
>
> The EFI functions that you see in this patch are for the runtime, i.e.
> after ExitBootServices().
>
> When you issue the reboot or poweroff command in Linux it will call the
> UEFI runtime. At this point all driver model code is gone. So we cannot
> call any DM U-Boot driver. This is why these two functions are marked as
> __efi_runtime.
The mark of __efi_runtime is troublesome in some cases. I remember on
x86 when booting from U-Boot EFI loader the Linux kernel calls EFI
runtime services would hang which is because we don't mark the
required functions with __efi_runtime in U-Boot. This is not a
scalable solution I think. In the end we may have to mark all U-Boot
functions to __efi_runtime.
> The RISC-V platform specification clearly states that you should use the
> SBI system reset if it exists.
>
> Before ExitBootServices() the U-Boot sysreset driver will be called
> which may be the SBI driver. U-Boot will iterate through all sysreset
> drivers until one actually resets the board.
>
> >
> > 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.
>
> On the Hifive Unmatched poweroff using OpenSBI just works fine. But
> OpenSBI does not have reset for the HiFive Unmatched due to a missing GPIO.
>
> I have no HiFive Unleashed for testing but calling OpenSBI should work
> out of the box as the device-tree defines the GPIOs both for reboot and
> poweroff that the reset driver looks for in OpenSBI. Do you have access
> to an Unleashed board for testing?
>
> For OpenSBI I created commit e928472e67f8 ("lib: utils: support both of
> gpio-poweroff, gpio-reset") which you will be needed for the Unleashed
> board. The patch is merged into upstream.
>
Regards,
Bin
