[PATCH 0/5] riscv: enable SBI system reset

[PATCH 0/5] riscv: enable SBI system reset

[Heinrich Schuchardt]

The purpose of this series is to provide the UEFI ResetSystem() service at
runtime on RISC-V systems.

With SBI v0.3 a system reset extension is available. This allows to
implement reboot and poweroff in U-Boot in a system independent way.

OpenSBI already provides an implementation of the extension for the
Kendryte K210 and other platforms.

* Provide a system reset driver using the system reset extension.
* Enable the driver on the MAIX board.
* Change the sbi command to use contants instead of raw numbers where
  applicable.

Heinrich Schuchardt (5):
  risv: add missing SBI extension definitions
  cmd/sbi: use constants instead of numerical values
  sysreset: provide SBI based sysreset driver
  pinctrl: K210_PINCTRL depends on REGMAP and on SYSCON
  maix: enable SBI system reset for MAIX

 MAINTAINERS                         |   1 +
 arch/riscv/include/asm/sbi.h        |  37 +++++++++-
 arch/riscv/lib/sbi.c                |  21 ++++--
 board/sipeed/maix/maix.c            |   4 ++
 cmd/riscv/sbi.c                     |  30 ++++----
 configs/sipeed_maix_smode_defconfig |   2 +
 drivers/pinctrl/Kconfig             |   2 +
 drivers/sysreset/Kconfig            |   7 ++
 drivers/sysreset/Makefile           |   1 +
 drivers/sysreset/sysreset_sbi.c     | 102 ++++++++++++++++++++++++++++
 lib/efi_loader/Kconfig              |   2 +-
 11 files changed, 187 insertions(+), 22 deletions(-)
 create mode 100644 drivers/sysreset/sysreset_sbi.c

--
2.30.1

[PATCH 1/5] risv: add missing SBI extension definitions

[Heinrich Schuchardt]

Add the System Reset Extension and the Hart State Management Extension
definitions.

Add missing RFENCE Extension enum values.

The SBI 0.1 extension constants are needed for for the sbi command. Remove
an #ifdef.

Cf. https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 arch/riscv/include/asm/sbi.h | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 53ca316180..c598bb11ce 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -12,7 +12,6 @@
 #include <linux/types.h>

 enum sbi_ext_id {
-#ifdef CONFIG_SBI_V01
 	SBI_EXT_0_1_SET_TIMER = 0x0,
 	SBI_EXT_0_1_CONSOLE_PUTCHAR = 0x1,
 	SBI_EXT_0_1_CONSOLE_GETCHAR = 0x2,
@@ -22,11 +21,12 @@ enum sbi_ext_id {
 	SBI_EXT_0_1_REMOTE_SFENCE_VMA = 0x6,
 	SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID = 0x7,
 	SBI_EXT_0_1_SHUTDOWN = 0x8,
-#endif
 	SBI_EXT_BASE = 0x10,
 	SBI_EXT_TIME = 0x54494D45,
 	SBI_EXT_IPI = 0x735049,
 	SBI_EXT_RFENCE = 0x52464E43,
+	SBI_EXT_HSM = 0x48534D,
+	SBI_EXT_SRST = 0x53525354,
 };

 enum sbi_ext_base_fid {
@@ -51,6 +51,38 @@ enum sbi_ext_rfence_fid {
 	SBI_EXT_RFENCE_REMOTE_FENCE_I = 0,
 	SBI_EXT_RFENCE_REMOTE_SFENCE_VMA,
 	SBI_EXT_RFENCE_REMOTE_SFENCE_VMA_ASID,
+	SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA,
+	SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA_VMID,
+	SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA,
+	SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA_ASID,
+};
+
+enum sbi_ext_hsm_fid {
+	SBI_EXT_HSM_HART_START = 0,
+	SBI_EXT_HSM_HART_STOP,
+	SBI_EXT_HSM_HART_STATUS,
+};
+
+enum sbi_hsm_hart_status {
+	SBI_HSM_HART_STATUS_STARTED = 0,
+	SBI_HSM_HART_STATUS_STOPPED,
+	SBI_HSM_HART_STATUS_START_PENDING,
+	SBI_HSM_HART_STATUS_STOP_PENDING,
+};
+
+enum sbi_ext_srst_fid {
+	SBI_EXT_SRST_RESET = 0,
+};
+
+enum sbi_srst_reset_type {
+	SBI_SRST_RESET_TYPE_SHUTDOWN = 0,
+	SBI_SRST_RESET_TYPE_COLD_REBOOT,
+	SBI_SRST_RESET_TYPE_WARM_REBOOT,
+};
+
+enum sbi_srst_reset_reason {
+	SBI_SRST_RESET_REASON_NONE = 0,
+	SBI_SRST_RESET_REASON_SYS_FAILURE,
 };

 #ifdef CONFIG_SBI_V01
--
2.30.1

[PATCH 2/5] cmd/sbi: use constants instead of numerical values

[Heinrich Schuchardt]

Use constants for extension IDs.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/riscv/sbi.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/cmd/riscv/sbi.c b/cmd/riscv/sbi.c
index 90c0811e14..65a2c93290 100644
--- a/cmd/riscv/sbi.c
+++ b/cmd/riscv/sbi.c
@@ -29,21 +29,21 @@ static struct sbi_imp implementations[] = {
 };

 static struct sbi_ext extensions[] = {
-	{ 0x00000000, "sbi_set_timer" },
-	{ 0x00000001, "sbi_console_putchar" },
-	{ 0x00000002, "sbi_console_getchar" },
-	{ 0x00000003, "sbi_clear_ipi" },
-	{ 0x00000004, "sbi_send_ipi" },
-	{ 0x00000005, "sbi_remote_fence_i" },
-	{ 0x00000006, "sbi_remote_sfence_vma" },
-	{ 0x00000007, "sbi_remote_sfence_vma_asid" },
-	{ 0x00000008, "sbi_shutdown" },
-	{ 0x00000010, "SBI Base Functionality" },
-	{ 0x54494D45, "Timer Extension" },
-	{ 0x00735049, "IPI Extension" },
-	{ 0x52464E43, "RFENCE Extension" },
-	{ 0x0048534D, "Hart State Management Extension" },
-	{ 0x53525354, "System Reset Extension" },
+	{ SBI_EXT_0_1_SET_TIMER,	      "sbi_set_timer" },
+	{ SBI_EXT_0_1_CONSOLE_PUTCHAR,	      "sbi_console_putchar" },
+	{ SBI_EXT_0_1_CONSOLE_GETCHAR,	      "sbi_console_getchar" },
+	{ SBI_EXT_0_1_CLEAR_IPI,	      "sbi_clear_ipi" },
+	{ SBI_EXT_0_1_SEND_IPI,		      "sbi_send_ipi" },
+	{ SBI_EXT_0_1_REMOTE_FENCE_I,	      "sbi_remote_fence_i" },
+	{ SBI_EXT_0_1_REMOTE_SFENCE_VMA,      "sbi_remote_sfence_vma" },
+	{ SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID, "sbi_remote_sfence_vma_asid" },
+	{ SBI_EXT_0_1_SHUTDOWN,		      "sbi_shutdown" },
+	{ SBI_EXT_BASE,			      "SBI Base Functionality" },
+	{ SBI_EXT_TIME,			      "Timer Extension" },
+	{ SBI_EXT_IPI,			      "IPI Extension" },
+	{ SBI_EXT_RFENCE,		      "RFENCE Extension" },
+	{ SBI_EXT_HSM,			      "Hart State Management Extension" },
+	{ SBI_EXT_SRST,			      "System Reset Extension" },
 };

 static int do_sbi(struct cmd_tbl *cmdtp, int flag, int argc,
--
2.30.1

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

[Heinrich Schuchardt]

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;
+
+	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"
--
2.30.1

[PATCH 4/5] pinctrl: K210_PINCTRL depends on REGMAP and on SYSCON

[Heinrich Schuchardt]

Select missing Kconfig dependencies.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 drivers/pinctrl/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 77fb851114..2a859ab5d0 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -294,6 +294,8 @@ config ASPEED_AST2500_PINCTRL
 config PINCTRL_K210
 	bool "Kendryte K210 Fully-Programmable Input/Output Array driver"
 	depends on DM && PINCTRL_GENERIC
+	select REGMAP
+	select SYSCON
 	help
 	  Support pin multiplexing on the K210. The "FPIOA" can remap any
 	  supported function to any multifunctional IO pin. It can also perform
--
2.30.1

[PATCH 5/5] maix: enable SBI system reset for MAIX

[Heinrich Schuchardt]

When running in S-mode we can use the SBI system reset extension to provide
the system reset.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 board/sipeed/maix/maix.c            | 5 +++++
 configs/sipeed_maix_smode_defconfig | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/board/sipeed/maix/maix.c b/board/sipeed/maix/maix.c
index cbcb23cf5c..388eddee6e 100644
--- a/board/sipeed/maix/maix.c
+++ b/board/sipeed/maix/maix.c
@@ -6,6 +6,7 @@
 #include <common.h>
 #include <clk.h>
 #include <dm.h>
+#include <dm/lists.h>
 #include <fdt_support.h>
 #include <asm/io.h>

@@ -37,5 +38,9 @@ int board_init(void)
 			return ret;
 	}

+	if (IS_ENABLED(CONFIG_SYSRESET_SBI))
+		ret = device_bind_driver(gd->dm_root, "sbi-sysreset",
+					 "sbi-sysreset", NULL);
+
 	return 0;
 }
diff --git a/configs/sipeed_maix_smode_defconfig b/configs/sipeed_maix_smode_defconfig
index 2516bb7258..aa95893feb 100644
--- a/configs/sipeed_maix_smode_defconfig
+++ b/configs/sipeed_maix_smode_defconfig
@@ -7,4 +7,6 @@ CONFIG_STACK_SIZE=0x100000
 # CONFIG_NET is not set
 # CONFIG_INPUT is not set
 # CONFIG_DM_ETH is not set
+CONFIG_SYSRESET_SBI=y
+# CONFIG_SYSRESET_SYSCON is not set
 # CONFIG_EFI_UNICODE_CAPITALIZATION is not set
--
2.30.1

[PATCH 4/5] pinctrl: K210_PINCTRL depends on REGMAP and on SYSCON

[Sean Anderson]

On 3/4/21 12:00 PM, Heinrich Schuchardt wrote:
> Select missing Kconfig dependencies.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   drivers/pinctrl/Kconfig | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 77fb851114..2a859ab5d0 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -294,6 +294,8 @@ config ASPEED_AST2500_PINCTRL
>   config PINCTRL_K210
>   	bool "Kendryte K210 Fully-Programmable Input/Output Array driver"
>   	depends on DM && PINCTRL_GENERIC
> +	select REGMAP
> +	select SYSCON
>   	help
>   	  Support pin multiplexing on the K210. The "FPIOA" can remap any
>   	  supported function to any multifunctional IO pin. It can also perform
> --
> 2.30.1
> 

Reviewed-by: Sean Anderson <seanga2@gmail.com>

[PATCH 5/5] maix: enable SBI system reset for MAIX

[Sean Anderson]

On 3/4/21 12:00 PM, Heinrich Schuchardt wrote:
> When running in S-mode we can use the SBI system reset extension to provide
> the system reset.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   board/sipeed/maix/maix.c            | 5 +++++
>   configs/sipeed_maix_smode_defconfig | 2 ++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/board/sipeed/maix/maix.c b/board/sipeed/maix/maix.c
> index cbcb23cf5c..388eddee6e 100644
> --- a/board/sipeed/maix/maix.c
> +++ b/board/sipeed/maix/maix.c
> @@ -6,6 +6,7 @@
>   #include <common.h>
>   #include <clk.h>
>   #include <dm.h>
> +#include <dm/lists.h>
>   #include <fdt_support.h>
>   #include <asm/io.h>
> 
> @@ -37,5 +38,9 @@ int board_init(void)
>   			return ret;
>   	}
> 
> +	if (IS_ENABLED(CONFIG_SYSRESET_SBI))
> +		ret = device_bind_driver(gd->dm_root, "sbi-sysreset",
> +					 "sbi-sysreset", NULL);
> +

Besides that this isn't a device-tree driver, shouldn't this live in
arch_early_init_r or similar? This isn't really board-specific.

--Sean

>   	return 0;
>   }
> diff --git a/configs/sipeed_maix_smode_defconfig b/configs/sipeed_maix_smode_defconfig
> index 2516bb7258..aa95893feb 100644
> --- a/configs/sipeed_maix_smode_defconfig
> +++ b/configs/sipeed_maix_smode_defconfig
> @@ -7,4 +7,6 @@ CONFIG_STACK_SIZE=0x100000
>   # CONFIG_NET is not set
>   # CONFIG_INPUT is not set
>   # CONFIG_DM_ETH is not set
> +CONFIG_SYSRESET_SBI=y
> +# CONFIG_SYSRESET_SYSCON is not set
>   # CONFIG_EFI_UNICODE_CAPITALIZATION is not set
> --
> 2.30.1
> 

[PATCH 1/5] risv: add missing SBI extension definitions

[Sean Anderson]

On 3/4/21 12:00 PM, Heinrich Schuchardt wrote:
> Add the System Reset Extension and the Hart State Management Extension
> definitions.
> 
> Add missing RFENCE Extension enum values.
> 
> The SBI 0.1 extension constants are needed for for the sbi command. Remove
> an #ifdef.
> 
> Cf. https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   arch/riscv/include/asm/sbi.h | 36 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 53ca316180..c598bb11ce 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -12,7 +12,6 @@
>   #include <linux/types.h>
> 
>   enum sbi_ext_id {
> -#ifdef CONFIG_SBI_V01
>   	SBI_EXT_0_1_SET_TIMER = 0x0,
>   	SBI_EXT_0_1_CONSOLE_PUTCHAR = 0x1,
>   	SBI_EXT_0_1_CONSOLE_GETCHAR = 0x2,
> @@ -22,11 +21,12 @@ enum sbi_ext_id {
>   	SBI_EXT_0_1_REMOTE_SFENCE_VMA = 0x6,
>   	SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID = 0x7,
>   	SBI_EXT_0_1_SHUTDOWN = 0x8,
> -#endif
>   	SBI_EXT_BASE = 0x10,
>   	SBI_EXT_TIME = 0x54494D45,
>   	SBI_EXT_IPI = 0x735049,
>   	SBI_EXT_RFENCE = 0x52464E43,
> +	SBI_EXT_HSM = 0x48534D,
> +	SBI_EXT_SRST = 0x53525354,
>   };
> 
>   enum sbi_ext_base_fid {
> @@ -51,6 +51,38 @@ enum sbi_ext_rfence_fid {
>   	SBI_EXT_RFENCE_REMOTE_FENCE_I = 0,
>   	SBI_EXT_RFENCE_REMOTE_SFENCE_VMA,
>   	SBI_EXT_RFENCE_REMOTE_SFENCE_VMA_ASID,
> +	SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA,
> +	SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA_VMID,
> +	SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA,
> +	SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA_ASID,

Isn't this backwards? That is, we should have

+	SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA_VMID,
+	SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA,
+	SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA_ASID,
+	SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA,

so the FIDs match.

--Sean

> +};
> +
> +enum sbi_ext_hsm_fid {
> +	SBI_EXT_HSM_HART_START = 0,
> +	SBI_EXT_HSM_HART_STOP,
> +	SBI_EXT_HSM_HART_STATUS,
> +};
> +
> +enum sbi_hsm_hart_status {
> +	SBI_HSM_HART_STATUS_STARTED = 0,
> +	SBI_HSM_HART_STATUS_STOPPED,
> +	SBI_HSM_HART_STATUS_START_PENDING,
> +	SBI_HSM_HART_STATUS_STOP_PENDING,
> +};
> +
> +enum sbi_ext_srst_fid {
> +	SBI_EXT_SRST_RESET = 0,
> +};
> +
> +enum sbi_srst_reset_type {
> +	SBI_SRST_RESET_TYPE_SHUTDOWN = 0,
> +	SBI_SRST_RESET_TYPE_COLD_REBOOT,
> +	SBI_SRST_RESET_TYPE_WARM_REBOOT,
> +};
> +
> +enum sbi_srst_reset_reason {
> +	SBI_SRST_RESET_REASON_NONE = 0,
> +	SBI_SRST_RESET_REASON_SYS_FAILURE,
>   };
> 
>   #ifdef CONFIG_SBI_V01
> --
> 2.30.1
> 

[PATCH 2/5] cmd/sbi: use constants instead of numerical values

[Sean Anderson]

On 3/4/21 12:00 PM, Heinrich Schuchardt wrote:
> Use constants for extension IDs.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   cmd/riscv/sbi.c | 30 +++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/cmd/riscv/sbi.c b/cmd/riscv/sbi.c
> index 90c0811e14..65a2c93290 100644
> --- a/cmd/riscv/sbi.c
> +++ b/cmd/riscv/sbi.c
> @@ -29,21 +29,21 @@ static struct sbi_imp implementations[] = {
>   };
> 
>   static struct sbi_ext extensions[] = {
> -	{ 0x00000000, "sbi_set_timer" },
> -	{ 0x00000001, "sbi_console_putchar" },
> -	{ 0x00000002, "sbi_console_getchar" },
> -	{ 0x00000003, "sbi_clear_ipi" },
> -	{ 0x00000004, "sbi_send_ipi" },
> -	{ 0x00000005, "sbi_remote_fence_i" },
> -	{ 0x00000006, "sbi_remote_sfence_vma" },
> -	{ 0x00000007, "sbi_remote_sfence_vma_asid" },
> -	{ 0x00000008, "sbi_shutdown" },
> -	{ 0x00000010, "SBI Base Functionality" },
> -	{ 0x54494D45, "Timer Extension" },
> -	{ 0x00735049, "IPI Extension" },
> -	{ 0x52464E43, "RFENCE Extension" },
> -	{ 0x0048534D, "Hart State Management Extension" },
> -	{ 0x53525354, "System Reset Extension" },
> +	{ SBI_EXT_0_1_SET_TIMER,	      "sbi_set_timer" },
> +	{ SBI_EXT_0_1_CONSOLE_PUTCHAR,	      "sbi_console_putchar" },
> +	{ SBI_EXT_0_1_CONSOLE_GETCHAR,	      "sbi_console_getchar" },
> +	{ SBI_EXT_0_1_CLEAR_IPI,	      "sbi_clear_ipi" },
> +	{ SBI_EXT_0_1_SEND_IPI,		      "sbi_send_ipi" },
> +	{ SBI_EXT_0_1_REMOTE_FENCE_I,	      "sbi_remote_fence_i" },
> +	{ SBI_EXT_0_1_REMOTE_SFENCE_VMA,      "sbi_remote_sfence_vma" },
> +	{ SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID, "sbi_remote_sfence_vma_asid" },
> +	{ SBI_EXT_0_1_SHUTDOWN,		      "sbi_shutdown" },
> +	{ SBI_EXT_BASE,			      "SBI Base Functionality" },
> +	{ SBI_EXT_TIME,			      "Timer Extension" },
> +	{ SBI_EXT_IPI,			      "IPI Extension" },
> +	{ SBI_EXT_RFENCE,		      "RFENCE Extension" },
> +	{ SBI_EXT_HSM,			      "Hart State Management Extension" },
> +	{ SBI_EXT_SRST,			      "System Reset Extension" },
>   };
> 
>   static int do_sbi(struct cmd_tbl *cmdtp, int flag, int argc,
> --
> 2.30.1
> 

Reviewed-by: Sean Anderson <seanga2@gmail.com>

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

[Sean Anderson]

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
> 

[PATCH 1/5] risv: add missing SBI extension definitions

[Heinrich Schuchardt]

On 3/5/21 12:28 AM, Sean Anderson wrote:
> On 3/4/21 12:00 PM, Heinrich Schuchardt wrote:
>> Add the System Reset Extension and the Hart State Management Extension
>> definitions.
>>
>> Add missing RFENCE Extension enum values.
>>
>> The SBI 0.1 extension constants are needed for for the sbi command.
>> Remove
>> an #ifdef.
>>
>> Cf. https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> ? arch/riscv/include/asm/sbi.h | 36 ++++++++++++++++++++++++++++++++++--
>> ? 1 file changed, 34 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
>> index 53ca316180..c598bb11ce 100644
>> --- a/arch/riscv/include/asm/sbi.h
>> +++ b/arch/riscv/include/asm/sbi.h
>> @@ -12,7 +12,6 @@
>> ? #include <linux/types.h>
>>
>> ? enum sbi_ext_id {
>> -#ifdef CONFIG_SBI_V01
>> ????? SBI_EXT_0_1_SET_TIMER = 0x0,
>> ????? SBI_EXT_0_1_CONSOLE_PUTCHAR = 0x1,
>> ????? SBI_EXT_0_1_CONSOLE_GETCHAR = 0x2,
>> @@ -22,11 +21,12 @@ enum sbi_ext_id {
>> ????? SBI_EXT_0_1_REMOTE_SFENCE_VMA = 0x6,
>> ????? SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID = 0x7,
>> ????? SBI_EXT_0_1_SHUTDOWN = 0x8,
>> -#endif
>> ????? SBI_EXT_BASE = 0x10,
>> ????? SBI_EXT_TIME = 0x54494D45,
>> ????? SBI_EXT_IPI = 0x735049,
>> ????? SBI_EXT_RFENCE = 0x52464E43,
>> +??? SBI_EXT_HSM = 0x48534D,
>> +??? SBI_EXT_SRST = 0x53525354,
>> ? };
>>
>> ? enum sbi_ext_base_fid {
>> @@ -51,6 +51,38 @@ enum sbi_ext_rfence_fid {
>> ????? SBI_EXT_RFENCE_REMOTE_FENCE_I = 0,
>> ????? SBI_EXT_RFENCE_REMOTE_SFENCE_VMA,
>> ????? SBI_EXT_RFENCE_REMOTE_SFENCE_VMA_ASID,
>> +??? SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA,
>> +??? SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA_VMID,
>> +??? SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA,
>> +??? SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA_ASID,
>
> Isn't this backwards? That is, we should have
>
> +??? SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA_VMID,
> +??? SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA,
> +??? SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA_ASID,
> +??? SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA,
>
> so the FIDs match.

You are right. I should not have copied from Linux without checking.

Cf.
https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc#78-function-listing

Best regards

Heinrich

>
> --Sean
>
>> +};
>> +
>> +enum sbi_ext_hsm_fid {
>> +??? SBI_EXT_HSM_HART_START = 0,
>> +??? SBI_EXT_HSM_HART_STOP,
>> +??? SBI_EXT_HSM_HART_STATUS,
>> +};
>> +
>> +enum sbi_hsm_hart_status {
>> +??? SBI_HSM_HART_STATUS_STARTED = 0,
>> +??? SBI_HSM_HART_STATUS_STOPPED,
>> +??? SBI_HSM_HART_STATUS_START_PENDING,
>> +??? SBI_HSM_HART_STATUS_STOP_PENDING,
>> +};
>> +
>> +enum sbi_ext_srst_fid {
>> +??? SBI_EXT_SRST_RESET = 0,
>> +};
>> +
>> +enum sbi_srst_reset_type {
>> +??? SBI_SRST_RESET_TYPE_SHUTDOWN = 0,
>> +??? SBI_SRST_RESET_TYPE_COLD_REBOOT,
>> +??? SBI_SRST_RESET_TYPE_WARM_REBOOT,
>> +};
>> +
>> +enum sbi_srst_reset_reason {
>> +??? SBI_SRST_RESET_REASON_NONE = 0,
>> +??? SBI_SRST_RESET_REASON_SYS_FAILURE,
>> ? };
>>
>> ? #ifdef CONFIG_SBI_V01
>> --
>> 2.30.1
>>
>

[PATCH 1/5] risv: add missing SBI extension definitions

[Bin Meng]

On Fri, Mar 5, 2021 at 1:02 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Add the System Reset Extension and the Hart State Management Extension
> definitions.
>
> Add missing RFENCE Extension enum values.
>
> The SBI 0.1 extension constants are needed for for the sbi command. Remove
> an #ifdef.
>
> Cf. https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  arch/riscv/include/asm/sbi.h | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
>

U-Boot does not need HSM.

Regards,
Bin

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

[Heinrich Schuchardt]

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

[PATCH 5/5] maix: enable SBI system reset for MAIX

[Heinrich Schuchardt]

On 3/5/21 12:24 AM, Sean Anderson wrote:
> On 3/4/21 12:00 PM, Heinrich Schuchardt wrote:
>> When running in S-mode we can use the SBI system reset extension to
>> provide
>> the system reset.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> ? board/sipeed/maix/maix.c??????????? | 5 +++++
>> ? configs/sipeed_maix_smode_defconfig | 2 ++
>> ? 2 files changed, 7 insertions(+)
>>
>> diff --git a/board/sipeed/maix/maix.c b/board/sipeed/maix/maix.c
>> index cbcb23cf5c..388eddee6e 100644
>> --- a/board/sipeed/maix/maix.c
>> +++ b/board/sipeed/maix/maix.c
>> @@ -6,6 +6,7 @@
>> ? #include <common.h>
>> ? #include <clk.h>
>> ? #include <dm.h>
>> +#include <dm/lists.h>
>> ? #include <fdt_support.h>
>> ? #include <asm/io.h>
>>
>> @@ -37,5 +38,9 @@ int board_init(void)
>> ????????????? return ret;
>> ????? }
>>
>> +??? if (IS_ENABLED(CONFIG_SYSRESET_SBI))
>> +??????? ret = device_bind_driver(gd->dm_root, "sbi-sysreset",
>> +???????????????????? "sbi-sysreset", NULL);
>> +
>
> Besides that this isn't a device-tree driver, shouldn't this live in
> arch_early_init_r or similar? This isn't really board-specific.

Using arch_early_init_r() seems to be a viable alternative to putting
some dummy node into each device-tree.

Best regards

Heinrich

>
> --Sean
>
>> ????? return 0;
>> ? }
>> diff --git a/configs/sipeed_maix_smode_defconfig
>> b/configs/sipeed_maix_smode_defconfig
>> index 2516bb7258..aa95893feb 100644
>> --- a/configs/sipeed_maix_smode_defconfig
>> +++ b/configs/sipeed_maix_smode_defconfig
>> @@ -7,4 +7,6 @@ CONFIG_STACK_SIZE=0x100000
>> ? # CONFIG_NET is not set
>> ? # CONFIG_INPUT is not set
>> ? # CONFIG_DM_ETH is not set
>> +CONFIG_SYSRESET_SBI=y
>> +# CONFIG_SYSRESET_SYSCON is not set
>> ? # CONFIG_EFI_UNICODE_CAPITALIZATION is not set
>> --
>> 2.30.1
>>
>

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

[Sean Anderson]

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

[PATCH 1/5] risv: add missing SBI extension definitions

[Sean Anderson]

On 3/6/21 12:59 AM, Bin Meng wrote:
> On Fri, Mar 5, 2021 at 1:02 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Add the System Reset Extension and the Hart State Management Extension
>> definitions.
>>
>> Add missing RFENCE Extension enum values.
>>
>> The SBI 0.1 extension constants are needed for for the sbi command. Remove
>> an #ifdef.
>>
>> Cf. https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   arch/riscv/include/asm/sbi.h | 36 ++++++++++++++++++++++++++++++++++--
>>   1 file changed, 34 insertions(+), 2 deletions(-)
>>
> 
> U-Boot does not need HSM.

(yet)

As far as I can tell, it's completely conforming for all harts but one
to be put into the STOPPED state by the SBI (with the expectation that
U-Boot or something else start them). Also, while we currently figure
out which harts to start via device tree passed from SBI, that behavior
is not standardized. Going forward, I we may see some SBI
implementations which communicate this sort of info through HSM. Though
this has not happened yet...

--Sean

[PATCH 2/5] cmd/sbi: use constants instead of numerical values

[Leo Liang]

On Thu, Mar 04, 2021 at 05:00:48PM +0000, Heinrich Schuchardt wrote:
> Use constants for extension IDs.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  cmd/riscv/sbi.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/cmd/riscv/sbi.c b/cmd/riscv/sbi.c
> index 90c0811e14..65a2c93290 100644
> --- a/cmd/riscv/sbi.c
> +++ b/cmd/riscv/sbi.c
> @@ -29,21 +29,21 @@ static struct sbi_imp implementations[] = {
>  };
> 
>  static struct sbi_ext extensions[] = {
> -	{ 0x00000000, "sbi_set_timer" },
> -	{ 0x00000001, "sbi_console_putchar" },
> -	{ 0x00000002, "sbi_console_getchar" },
> -	{ 0x00000003, "sbi_clear_ipi" },
> -	{ 0x00000004, "sbi_send_ipi" },
> -	{ 0x00000005, "sbi_remote_fence_i" },
> -	{ 0x00000006, "sbi_remote_sfence_vma" },
> -	{ 0x00000007, "sbi_remote_sfence_vma_asid" },
> -	{ 0x00000008, "sbi_shutdown" },
> -	{ 0x00000010, "SBI Base Functionality" },
> -	{ 0x54494D45, "Timer Extension" },
> -	{ 0x00735049, "IPI Extension" },
> -	{ 0x52464E43, "RFENCE Extension" },
> -	{ 0x0048534D, "Hart State Management Extension" },
> -	{ 0x53525354, "System Reset Extension" },
> +	{ SBI_EXT_0_1_SET_TIMER,	      "sbi_set_timer" },
> +	{ SBI_EXT_0_1_CONSOLE_PUTCHAR,	      "sbi_console_putchar" },
> +	{ SBI_EXT_0_1_CONSOLE_GETCHAR,	      "sbi_console_getchar" },
> +	{ SBI_EXT_0_1_CLEAR_IPI,	      "sbi_clear_ipi" },
> +	{ SBI_EXT_0_1_SEND_IPI,		      "sbi_send_ipi" },
> +	{ SBI_EXT_0_1_REMOTE_FENCE_I,	      "sbi_remote_fence_i" },
> +	{ SBI_EXT_0_1_REMOTE_SFENCE_VMA,      "sbi_remote_sfence_vma" },
> +	{ SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID, "sbi_remote_sfence_vma_asid" },
> +	{ SBI_EXT_0_1_SHUTDOWN,		      "sbi_shutdown" },
> +	{ SBI_EXT_BASE,			      "SBI Base Functionality" },
> +	{ SBI_EXT_TIME,			      "Timer Extension" },
> +	{ SBI_EXT_IPI,			      "IPI Extension" },
> +	{ SBI_EXT_RFENCE,		      "RFENCE Extension" },
> +	{ SBI_EXT_HSM,			      "Hart State Management Extension" },
> +	{ SBI_EXT_SRST,			      "System Reset Extension" },
>  };
> 
>  static int do_sbi(struct cmd_tbl *cmdtp, int flag, int argc,
> --
> 2.30.1
>

Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>

[PATCH 4/5] pinctrl: K210_PINCTRL depends on REGMAP and on SYSCON

[Leo Liang]

On Thu, Mar 04, 2021 at 05:00:50PM +0000, Heinrich Schuchardt wrote:
> Select missing Kconfig dependencies.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/pinctrl/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 77fb851114..2a859ab5d0 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -294,6 +294,8 @@ config ASPEED_AST2500_PINCTRL
>  config PINCTRL_K210
>  	bool "Kendryte K210 Fully-Programmable Input/Output Array driver"
>  	depends on DM && PINCTRL_GENERIC
> +	select REGMAP
> +	select SYSCON
>  	help
>  	  Support pin multiplexing on the K210. The "FPIOA" can remap any
>  	  supported function to any multifunctional IO pin. It can also perform
> --
> 2.30.1
>

Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>