[PATCH 00/11] SBI system suspend (SUSP) extension

[PATCH 00/11] SBI system suspend (SUSP) extension

[Andrew Jones]

This series implements the SBI system suspend support in OpenSBI
as-per the draft proposal for SBI system suspend which can be found at
https://github.com/jones-drew/riscv-sbi-doc/commit/d9e43e9a938fc3eb510e023c3f352462876f7785

The first half of the series are a few cleanups and fixes for HSM,
as system suspend makes use of some of its code and patterns. The
second half implements support for the extension and includes a patch
which allows testing to be enabled, even when low-level firmware does
not provide support. Regarding the test mode, it's currently enabled
by adding a 'system-suspend-test' DT property, but Anup has suggested
that FW_OPTION could also be used. I'm all ears for what people think
the best way to control test functionality in OpenSBI is and am happy
to switch it. To test with Linux the kernel must include the patch at
https://github.com/jones-drew/linux/commits/riscv/sbi-susp-rfc and
be built with CONFIG_SUSPEND enabled.

The patches are based on commit da5594bf853d ("platform: generic:
allwinner: Fix PLIC array bounds") and can also be found at
https://github.com/jones-drew/opensbi/commits/susp-v1

v2:
 - Rebased on latest master
 - Added a print line to sbi_boot_print_general() [Anup]
 - Picked up Anup's r-b's

Andrew Jones (11):
  lib: sbi_hsm: Factor out invalid state detection
  lib: sbi_hsm: Don't try to restore state on failed change
  lib: sbi_hsm: Ensure errors are consistent with spec
  lib: sbi_hsm: Move misplaced comment
  lib: sbi_hsm: Remove unnecessary include
  lib: sbi_hsm: Export some functions
  lib: sbi: Add system suspend skeleton
  lib: sbi: Add system_suspend_allowed domain property
  lib: sbi: Implement system suspend
  docs: Correct opensbi-domain property name
  platform: generic: Add system suspend test

 docs/domain_support.md            |  15 +++-
 include/sbi/sbi_domain.h          |   2 +
 include/sbi/sbi_ecall_interface.h |   8 +++
 include/sbi/sbi_hsm.h             |   4 ++
 include/sbi/sbi_system.h          |  18 +++++
 lib/sbi/Kconfig                   |   4 ++
 lib/sbi/objects.mk                |   3 +
 lib/sbi/sbi_domain.c              |   4 ++
 lib/sbi/sbi_ecall_hsm.c           |   4 +-
 lib/sbi/sbi_ecall_susp.c          |  48 +++++++++++++
 lib/sbi/sbi_hsm.c                 |  92 +++++++++++--------------
 lib/sbi/sbi_init.c                |   4 ++
 lib/sbi/sbi_system.c              | 111 ++++++++++++++++++++++++++++++
 lib/utils/fdt/fdt_domain.c        |   7 ++
 platform/generic/platform.c       |  20 +++++-
 15 files changed, 287 insertions(+), 57 deletions(-)
 create mode 100644 lib/sbi/sbi_ecall_susp.c

-- 
2.39.0

[PATCH 01/11] lib: sbi_hsm: Factor out invalid state detection

[Andrew Jones]

Remove some redundant code by creating an invalid state detection
macro.

No functional change intended.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 lib/sbi/sbi_hsm.c | 65 +++++++++++++++++------------------------------
 1 file changed, 24 insertions(+), 41 deletions(-)

diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c
index c0a5505423bf..b43f26cfe3a0 100644
--- a/lib/sbi/sbi_hsm.c
+++ b/lib/sbi/sbi_hsm.c
@@ -26,6 +26,15 @@
 #include <sbi/sbi_timer.h>
 #include <sbi/sbi_console.h>
 
+#define __sbi_hsm_hart_change_state(hdata, oldstate, newstate)		\
+({									\
+	long state = atomic_cmpxchg(&(hdata)->state, oldstate, newstate); \
+	if (state != (oldstate))					\
+		sbi_printf("%s: ERR: The hart is in invalid state [%lu]\n", \
+			   __func__, state);				\
+	state == (oldstate);						\
+})
+
 static const struct sbi_hsm_device *hsm_dev = NULL;
 static unsigned long hart_data_offset;
 
@@ -95,13 +104,11 @@ int sbi_hsm_hart_interruptible_mask(const struct sbi_domain *dom,
 
 void sbi_hsm_prepare_next_jump(struct sbi_scratch *scratch, u32 hartid)
 {
-	u32 oldstate;
 	struct sbi_hsm_data *hdata = sbi_scratch_offset_ptr(scratch,
 							    hart_data_offset);
 
-	oldstate = atomic_cmpxchg(&hdata->state, SBI_HSM_STATE_START_PENDING,
-				  SBI_HSM_STATE_STARTED);
-	if (oldstate != SBI_HSM_STATE_START_PENDING)
+	if (!__sbi_hsm_hart_change_state(hdata, SBI_HSM_STATE_START_PENDING,
+					 SBI_HSM_STATE_STARTED))
 		sbi_hart_hang();
 }
 
@@ -217,14 +224,12 @@ int sbi_hsm_init(struct sbi_scratch *scratch, u32 hartid, bool cold_boot)
 
 void __noreturn sbi_hsm_exit(struct sbi_scratch *scratch)
 {
-	u32 hstate;
 	struct sbi_hsm_data *hdata = sbi_scratch_offset_ptr(scratch,
 							    hart_data_offset);
 	void (*jump_warmboot)(void) = (void (*)(void))scratch->warmboot_addr;
 
-	hstate = atomic_cmpxchg(&hdata->state, SBI_HSM_STATE_STOP_PENDING,
-				SBI_HSM_STATE_STOPPED);
-	if (hstate != SBI_HSM_STATE_STOP_PENDING)
+	if (!__sbi_hsm_hart_change_state(hdata, SBI_HSM_STATE_STOP_PENDING,
+					 SBI_HSM_STATE_STOPPED))
 		goto fail_exit;
 
 	if (hsm_device_has_hart_hotplug()) {
@@ -299,7 +304,6 @@ int sbi_hsm_hart_start(struct sbi_scratch *scratch,
 
 int sbi_hsm_hart_stop(struct sbi_scratch *scratch, bool exitnow)
 {
-	int oldstate;
 	const struct sbi_domain *dom = sbi_domain_thishart_ptr();
 	struct sbi_hsm_data *hdata = sbi_scratch_offset_ptr(scratch,
 							    hart_data_offset);
@@ -307,13 +311,9 @@ int sbi_hsm_hart_stop(struct sbi_scratch *scratch, bool exitnow)
 	if (!dom)
 		return SBI_EFAIL;
 
-	oldstate = atomic_cmpxchg(&hdata->state, SBI_HSM_STATE_STARTED,
-				  SBI_HSM_STATE_STOP_PENDING);
-	if (oldstate != SBI_HSM_STATE_STARTED) {
-		sbi_printf("%s: ERR: The hart is in invalid state [%u]\n",
-			   __func__, oldstate);
+	if (!__sbi_hsm_hart_change_state(hdata, SBI_HSM_STATE_STARTED,
+					 SBI_HSM_STATE_STOP_PENDING))
 		return SBI_EFAIL;
-	}
 
 	if (exitnow)
 		sbi_exit(scratch);
@@ -363,36 +363,26 @@ static void __sbi_hsm_suspend_non_ret_restore(struct sbi_scratch *scratch)
 
 void sbi_hsm_hart_resume_start(struct sbi_scratch *scratch)
 {
-	int oldstate;
 	struct sbi_hsm_data *hdata = sbi_scratch_offset_ptr(scratch,
 							    hart_data_offset);
 
 	/* If current HART was SUSPENDED then set RESUME_PENDING state */
-	oldstate = atomic_cmpxchg(&hdata->state, SBI_HSM_STATE_SUSPENDED,
-			SBI_HSM_STATE_RESUME_PENDING);
-	if (oldstate != SBI_HSM_STATE_SUSPENDED) {
-		sbi_printf("%s: ERR: The hart is in invalid state [%u]\n",
-			   __func__, oldstate);
+	if (!__sbi_hsm_hart_change_state(hdata, SBI_HSM_STATE_SUSPENDED,
+					 SBI_HSM_STATE_RESUME_PENDING))
 		sbi_hart_hang();
-	}
 
 	hsm_device_hart_resume();
 }
 
 void sbi_hsm_hart_resume_finish(struct sbi_scratch *scratch)
 {
-	u32 oldstate;
 	struct sbi_hsm_data *hdata = sbi_scratch_offset_ptr(scratch,
 							    hart_data_offset);
 
 	/* If current HART was RESUME_PENDING then set STARTED state */
-	oldstate = atomic_cmpxchg(&hdata->state, SBI_HSM_STATE_RESUME_PENDING,
-				  SBI_HSM_STATE_STARTED);
-	if (oldstate != SBI_HSM_STATE_RESUME_PENDING) {
-		sbi_printf("%s: ERR: The hart is in invalid state [%u]\n",
-			   __func__, oldstate);
+	if (!__sbi_hsm_hart_change_state(hdata, SBI_HSM_STATE_RESUME_PENDING,
+					 SBI_HSM_STATE_STARTED))
 		sbi_hart_hang();
-	}
 
 	/*
 	 * Restore some of the M-mode CSRs which we are re-configured by
@@ -404,7 +394,7 @@ void sbi_hsm_hart_resume_finish(struct sbi_scratch *scratch)
 int sbi_hsm_hart_suspend(struct sbi_scratch *scratch, u32 suspend_type,
 			 ulong raddr, ulong rmode, ulong arg1)
 {
-	int oldstate, ret;
+	int ret;
 	const struct sbi_domain *dom = sbi_domain_thishart_ptr();
 	struct sbi_hsm_data *hdata = sbi_scratch_offset_ptr(scratch,
 							    hart_data_offset);
@@ -438,11 +428,8 @@ int sbi_hsm_hart_suspend(struct sbi_scratch *scratch, u32 suspend_type,
 	scratch->next_mode = rmode;
 
 	/* Directly move from STARTED to SUSPENDED state */
-	oldstate = atomic_cmpxchg(&hdata->state, SBI_HSM_STATE_STARTED,
-				  SBI_HSM_STATE_SUSPENDED);
-	if (oldstate != SBI_HSM_STATE_STARTED) {
-		sbi_printf("%s: ERR: The hart is in invalid state [%u]\n",
-			   __func__, oldstate);
+	if (!__sbi_hsm_hart_change_state(hdata, SBI_HSM_STATE_STARTED,
+					 SBI_HSM_STATE_SUSPENDED)) {
 		ret = SBI_EDENIED;
 		goto fail_restore_state;
 	}
@@ -485,13 +472,9 @@ fail_restore_state:
 	 * We might have successfully resumed from retentive suspend
 	 * or suspend failed. In both cases, we restore state of hart.
 	 */
-	oldstate = atomic_cmpxchg(&hdata->state, SBI_HSM_STATE_SUSPENDED,
-				  SBI_HSM_STATE_STARTED);
-	if (oldstate != SBI_HSM_STATE_SUSPENDED) {
-		sbi_printf("%s: ERR: The hart is in invalid state [%u]\n",
-			   __func__, oldstate);
+	if (!__sbi_hsm_hart_change_state(hdata, SBI_HSM_STATE_SUSPENDED,
+					 SBI_HSM_STATE_STARTED))
 		sbi_hart_hang();
-	}
 
 	return ret;
 }
-- 
2.39.0

[PATCH 02/11] lib: sbi_hsm: Don't try to restore state on failed change

[Andrew Jones]

When a state change fails there's no need to restore the original
state as it remains the same.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 lib/sbi/sbi_hsm.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c
index b43f26cfe3a0..c7454499b2ba 100644
--- a/lib/sbi/sbi_hsm.c
+++ b/lib/sbi/sbi_hsm.c
@@ -429,10 +429,8 @@ int sbi_hsm_hart_suspend(struct sbi_scratch *scratch, u32 suspend_type,
 
 	/* Directly move from STARTED to SUSPENDED state */
 	if (!__sbi_hsm_hart_change_state(hdata, SBI_HSM_STATE_STARTED,
-					 SBI_HSM_STATE_SUSPENDED)) {
-		ret = SBI_EDENIED;
-		goto fail_restore_state;
-	}
+					 SBI_HSM_STATE_SUSPENDED))
+		return SBI_EDENIED;
 
 	/* Save the suspend type */
 	hdata->suspend_type = suspend_type;
@@ -467,7 +465,6 @@ int sbi_hsm_hart_suspend(struct sbi_scratch *scratch, u32 suspend_type,
 		jump_warmboot();
 	}
 
-fail_restore_state:
 	/*
 	 * We might have successfully resumed from retentive suspend
 	 * or suspend failed. In both cases, we restore state of hart.
-- 
2.39.0

[PATCH 03/11] lib: sbi_hsm: Ensure errors are consistent with spec

[Andrew Jones]

HSM functions define when SBI_ERR_INVALID_PARAM should be returned.
Ensure it's not used for reasons that don't meet the definitions by
using the catch-all code, SBI_ERR_FAILED, for those reasons instead.
Also, in one case sbi_hart_suspend() may have returned SBI_ERR_DENIED,
which isn't defined for that function at all. Use SBI_ERR_FAILED for
that case too.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 lib/sbi/sbi_hsm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c
index c7454499b2ba..2fbb0d0afc7e 100644
--- a/lib/sbi/sbi_hsm.c
+++ b/lib/sbi/sbi_hsm.c
@@ -403,7 +403,7 @@ int sbi_hsm_hart_suspend(struct sbi_scratch *scratch, u32 suspend_type,
 
 	/* Sanity check on domain assigned to current HART */
 	if (!dom)
-		return SBI_EINVAL;
+		return SBI_EFAIL;
 
 	/* Sanity check on suspend type */
 	if (SBI_HSM_SUSPEND_RET_DEFAULT < suspend_type &&
@@ -416,7 +416,7 @@ int sbi_hsm_hart_suspend(struct sbi_scratch *scratch, u32 suspend_type,
 	/* Additional sanity check for non-retentive suspend */
 	if (suspend_type & SBI_HSM_SUSP_NON_RET_BIT) {
 		if (rmode != PRV_S && rmode != PRV_U)
-			return SBI_EINVAL;
+			return SBI_EFAIL;
 		if (dom && !sbi_domain_check_addr(dom, raddr, rmode,
 						  SBI_DOMAIN_EXECUTE))
 			return SBI_EINVALID_ADDR;
@@ -430,7 +430,7 @@ int sbi_hsm_hart_suspend(struct sbi_scratch *scratch, u32 suspend_type,
 	/* Directly move from STARTED to SUSPENDED state */
 	if (!__sbi_hsm_hart_change_state(hdata, SBI_HSM_STATE_STARTED,
 					 SBI_HSM_STATE_SUSPENDED))
-		return SBI_EDENIED;
+		return SBI_EFAIL;
 
 	/* Save the suspend type */
 	hdata->suspend_type = suspend_type;
-- 
2.39.0

[PATCH 04/11] lib: sbi_hsm: Move misplaced comment

[Andrew Jones]

While non-retentive suspend is not allowed for M-mode, the comment
at the top of sbi_hsm_hart_suspend() implied suspend wasn't allowed
for M-mode at all. Move the comment above the mode check which is
inside a suspend type is non-retentive check.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 lib/sbi/sbi_hsm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c
index 2fbb0d0afc7e..2eb189a9d7f3 100644
--- a/lib/sbi/sbi_hsm.c
+++ b/lib/sbi/sbi_hsm.c
@@ -399,8 +399,6 @@ int sbi_hsm_hart_suspend(struct sbi_scratch *scratch, u32 suspend_type,
 	struct sbi_hsm_data *hdata = sbi_scratch_offset_ptr(scratch,
 							    hart_data_offset);
 
-	/* For now, we only allow suspend from S-mode or U-mode. */
-
 	/* Sanity check on domain assigned to current HART */
 	if (!dom)
 		return SBI_EFAIL;
@@ -415,6 +413,10 @@ int sbi_hsm_hart_suspend(struct sbi_scratch *scratch, u32 suspend_type,
 
 	/* Additional sanity check for non-retentive suspend */
 	if (suspend_type & SBI_HSM_SUSP_NON_RET_BIT) {
+		/*
+		 * For now, we only allow non-retentive suspend from
+		 * S-mode or U-mode.
+		 */
 		if (rmode != PRV_S && rmode != PRV_U)
 			return SBI_EFAIL;
 		if (dom && !sbi_domain_check_addr(dom, raddr, rmode,
-- 
2.39.0

[PATCH 05/11] lib: sbi_hsm: Remove unnecessary include

[Andrew Jones]

Also remove a superfluous semicolon and add a blank line.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 lib/sbi/sbi_ecall_hsm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/sbi/sbi_ecall_hsm.c b/lib/sbi/sbi_ecall_hsm.c
index 703a824493b4..f1b41d073b22 100644
--- a/lib/sbi/sbi_ecall_hsm.c
+++ b/lib/sbi/sbi_ecall_hsm.c
@@ -12,7 +12,6 @@
 #include <sbi/sbi_ecall_interface.h>
 #include <sbi/sbi_error.h>
 #include <sbi/sbi_trap.h>
-#include <sbi/sbi_version.h>
 #include <sbi/sbi_hsm.h>
 #include <sbi/sbi_scratch.h>
 #include <sbi/riscv_asm.h>
@@ -45,7 +44,8 @@ static int sbi_ecall_hsm_handler(unsigned long extid, unsigned long funcid,
 		break;
 	default:
 		ret = SBI_ENOTSUPP;
-	};
+	}
+
 	if (ret >= 0) {
 		*out_val = ret;
 		ret = 0;
-- 
2.39.0

[PATCH 06/11] lib: sbi_hsm: Export some functions

[Andrew Jones]

A coming patch can make use of a few internal hsm functions if
we export them.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 include/sbi/sbi_hsm.h |  4 ++++
 lib/sbi/sbi_hsm.c     | 12 ++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/sbi/sbi_hsm.h b/include/sbi/sbi_hsm.h
index c2a728a4aca5..c730db1810df 100644
--- a/include/sbi/sbi_hsm.h
+++ b/include/sbi/sbi_hsm.h
@@ -65,9 +65,13 @@ void sbi_hsm_hart_resume_start(struct sbi_scratch *scratch);
 void sbi_hsm_hart_resume_finish(struct sbi_scratch *scratch);
 int sbi_hsm_hart_suspend(struct sbi_scratch *scratch, u32 suspend_type,
 			 ulong raddr, ulong rmode, ulong arg1);
+bool sbi_hsm_hart_change_state(struct sbi_scratch *scratch, long oldstate,
+			       long newstate);
+int __sbi_hsm_hart_get_state(u32 hartid);
 int sbi_hsm_hart_get_state(const struct sbi_domain *dom, u32 hartid);
 int sbi_hsm_hart_interruptible_mask(const struct sbi_domain *dom,
 				    ulong hbase, ulong *out_hmask);
+void __sbi_hsm_suspend_non_ret_save(struct sbi_scratch *scratch);
 void sbi_hsm_prepare_next_jump(struct sbi_scratch *scratch, u32 hartid);
 
 #endif
diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c
index 2eb189a9d7f3..3adedff8f232 100644
--- a/lib/sbi/sbi_hsm.c
+++ b/lib/sbi/sbi_hsm.c
@@ -46,7 +46,15 @@ struct sbi_hsm_data {
 	unsigned long saved_mip;
 };
 
-static inline int __sbi_hsm_hart_get_state(u32 hartid)
+bool sbi_hsm_hart_change_state(struct sbi_scratch *scratch, long oldstate,
+			       long newstate)
+{
+	struct sbi_hsm_data *hdata = sbi_scratch_offset_ptr(scratch,
+							    hart_data_offset);
+	return __sbi_hsm_hart_change_state(hdata, oldstate, newstate);
+}
+
+int __sbi_hsm_hart_get_state(u32 hartid)
 {
 	struct sbi_hsm_data *hdata;
 	struct sbi_scratch *scratch;
@@ -329,7 +337,7 @@ static int __sbi_hsm_suspend_default(struct sbi_scratch *scratch)
 	return 0;
 }
 
-static void __sbi_hsm_suspend_non_ret_save(struct sbi_scratch *scratch)
+void __sbi_hsm_suspend_non_ret_save(struct sbi_scratch *scratch)
 {
 	struct sbi_hsm_data *hdata = sbi_scratch_offset_ptr(scratch,
 							    hart_data_offset);
-- 
2.39.0

[PATCH 07/11] lib: sbi: Add system suspend skeleton

[Andrew Jones]

Add the SUSP extension probe and ecall support, but for now the
system suspend function is just a stub.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 include/sbi/sbi_ecall_interface.h |  8 ++++++
 include/sbi/sbi_system.h          | 17 +++++++++++
 lib/sbi/Kconfig                   |  4 +++
 lib/sbi/objects.mk                |  3 ++
 lib/sbi/sbi_ecall_susp.c          | 48 +++++++++++++++++++++++++++++++
 lib/sbi/sbi_init.c                |  4 +++
 lib/sbi/sbi_system.c              | 26 +++++++++++++++++
 7 files changed, 110 insertions(+)
 create mode 100644 lib/sbi/sbi_ecall_susp.c

diff --git a/include/sbi/sbi_ecall_interface.h b/include/sbi/sbi_ecall_interface.h
index a3f2bf4bdf69..98a426a74ec4 100644
--- a/include/sbi/sbi_ecall_interface.h
+++ b/include/sbi/sbi_ecall_interface.h
@@ -29,6 +29,7 @@
 #define SBI_EXT_HSM				0x48534D
 #define SBI_EXT_SRST				0x53525354
 #define SBI_EXT_PMU				0x504D55
+#define SBI_EXT_SUSP				0x53555350
 
 /* SBI function IDs for BASE extension*/
 #define SBI_EXT_BASE_GET_SPEC_VERSION		0x0
@@ -230,6 +231,13 @@ enum sbi_pmu_ctr_type {
 /* Flags defined for counter stop function */
 #define SBI_PMU_STOP_FLAG_RESET (1 << 0)
 
+/* SBI function IDs for SUSP extension */
+#define SBI_EXT_SUSP_SUSPEND			0x0
+
+#define SBI_SUSP_SLEEP_TYPE_SUSPEND		0x0
+#define SBI_SUSP_SLEEP_TYPE_LAST		SBI_SUSP_SLEEP_TYPE_SUSPEND
+#define SBI_SUSP_PLATFORM_SLEEP_START		0x80000000
+
 /* SBI base specification related macros */
 #define SBI_SPEC_VERSION_MAJOR_OFFSET		24
 #define SBI_SPEC_VERSION_MAJOR_MASK		0x7f
diff --git a/include/sbi/sbi_system.h b/include/sbi/sbi_system.h
index 84c281383d8c..65ea3d36d6cf 100644
--- a/include/sbi/sbi_system.h
+++ b/include/sbi/sbi_system.h
@@ -43,4 +43,21 @@ bool sbi_system_reset_supported(u32 reset_type, u32 reset_reason);
 
 void __noreturn sbi_system_reset(u32 reset_type, u32 reset_reason);
 
+/** System suspend device */
+struct sbi_system_suspend_device {
+	/** Name of the system suspend device */
+	char name[32];
+
+	/* Check whether sleep type is supported by the device */
+	int (*system_suspend_check)(u32 sleep_type);
+
+	/** Suspend the system */
+	int (*system_suspend)(u32 sleep_type);
+};
+
+const struct sbi_system_suspend_device *sbi_system_suspend_get_device(void);
+void sbi_system_suspend_set_device(struct sbi_system_suspend_device *dev);
+bool sbi_system_suspend_supported(u32 sleep_type);
+int sbi_system_suspend(u32 sleep_type, ulong resume_addr, ulong opaque);
+
 #endif
diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
index df74bba38540..7e139017fec7 100644
--- a/lib/sbi/Kconfig
+++ b/lib/sbi/Kconfig
@@ -22,6 +22,10 @@ config SBI_ECALL_SRST
 	bool "System Reset extension"
 	default y
 
+config SBI_ECALL_SUSP
+	bool "System Suspend extension"
+	default y
+
 config SBI_ECALL_PMU
 	bool "Performance Monitoring Unit extension"
 	default y
diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
index c774ebbcd142..c4f3d0991aa2 100644
--- a/lib/sbi/objects.mk
+++ b/lib/sbi/objects.mk
@@ -34,6 +34,9 @@ libsbi-objs-$(CONFIG_SBI_ECALL_HSM) += sbi_ecall_hsm.o
 carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_SRST) += ecall_srst
 libsbi-objs-$(CONFIG_SBI_ECALL_SRST) += sbi_ecall_srst.o
 
+carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_SUSP) += ecall_susp
+libsbi-objs-$(CONFIG_SBI_ECALL_SUSP) += sbi_ecall_susp.o
+
 carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_PMU) += ecall_pmu
 libsbi-objs-$(CONFIG_SBI_ECALL_PMU) += sbi_ecall_pmu.o
 
diff --git a/lib/sbi/sbi_ecall_susp.c b/lib/sbi/sbi_ecall_susp.c
new file mode 100644
index 000000000000..f20126c49a60
--- /dev/null
+++ b/lib/sbi/sbi_ecall_susp.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: BSD-2-Clause
+#include <sbi/sbi_ecall.h>
+#include <sbi/sbi_ecall_interface.h>
+#include <sbi/sbi_error.h>
+#include <sbi/sbi_trap.h>
+#include <sbi/sbi_system.h>
+
+static int sbi_ecall_susp_handler(unsigned long extid, unsigned long funcid,
+				  const struct sbi_trap_regs *regs,
+				  unsigned long *out_val,
+				  struct sbi_trap_info *out_trap)
+{
+	int ret = SBI_ENOTSUPP;
+
+	if (funcid == SBI_EXT_SUSP_SUSPEND)
+		ret = sbi_system_suspend(regs->a0, regs->a1, regs->a2);
+
+	if (ret >= 0) {
+		*out_val = ret;
+		ret = 0;
+	}
+
+	return ret;
+}
+
+static int sbi_ecall_susp_probe(unsigned long extid, unsigned long *out_val)
+{
+	u32 type, count = 0;
+
+	/*
+	 * At least one suspend type should be supported by the
+	 * platform for the SBI SUSP extension to be usable.
+	 */
+	for (type = 0; type <= SBI_SUSP_SLEEP_TYPE_LAST; type++) {
+		if (sbi_system_suspend_supported(type))
+			count++;
+	}
+
+	*out_val = count ? 1 : 0;
+	return 0;
+}
+
+struct sbi_ecall_extension ecall_susp = {
+	.extid_start = SBI_EXT_SUSP,
+	.extid_end = SBI_EXT_SUSP,
+	.handle = sbi_ecall_susp_handler,
+	.probe = sbi_ecall_susp_probe,
+};
diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
index 259a191051a7..0d8fdfbe9a41 100644
--- a/lib/sbi/sbi_init.c
+++ b/lib/sbi/sbi_init.c
@@ -69,6 +69,7 @@ static void sbi_boot_print_general(struct sbi_scratch *scratch)
 	const struct sbi_timer_device *tdev;
 	const struct sbi_console_device *cdev;
 	const struct sbi_system_reset_device *srdev;
+	const struct sbi_system_suspend_device *susp_dev;
 	const struct sbi_platform *plat = sbi_platform_ptr(scratch);
 
 	if (scratch->options & SBI_SCRATCH_NO_BOOT_PRINTS)
@@ -103,6 +104,9 @@ static void sbi_boot_print_general(struct sbi_scratch *scratch)
 	srdev = sbi_system_reset_get_device(SBI_SRST_RESET_TYPE_SHUTDOWN, 0);
 	sbi_printf("Platform Shutdown Device  : %s\n",
 		   (srdev) ? srdev->name : "---");
+	susp_dev = sbi_system_suspend_get_device();
+	sbi_printf("Platform Suspend Device   : %s\n",
+		   (susp_dev) ? susp_dev->name : "---");
 
 	/* Firmware details */
 	sbi_printf("Firmware Base             : 0x%lx\n", scratch->fw_start);
diff --git a/lib/sbi/sbi_system.c b/lib/sbi/sbi_system.c
index f37c811d0bf3..5c123a6c9d8d 100644
--- a/lib/sbi/sbi_system.c
+++ b/lib/sbi/sbi_system.c
@@ -92,3 +92,29 @@ void __noreturn sbi_system_reset(u32 reset_type, u32 reset_reason)
 	/* If platform specific reset did not work then do sbi_exit() */
 	sbi_exit(scratch);
 }
+
+static const struct sbi_system_suspend_device *suspend_dev = NULL;
+
+const struct sbi_system_suspend_device *sbi_system_suspend_get_device(void)
+{
+	return suspend_dev;
+}
+
+void sbi_system_suspend_set_device(struct sbi_system_suspend_device *dev)
+{
+	if (!dev || suspend_dev)
+		return;
+
+	suspend_dev = dev;
+}
+
+bool sbi_system_suspend_supported(u32 sleep_type)
+{
+	return suspend_dev && suspend_dev->system_suspend_check &&
+	       suspend_dev->system_suspend_check(sleep_type);
+}
+
+int sbi_system_suspend(u32 sleep_type, ulong resume_addr, ulong opaque)
+{
+	return 0;
+}
-- 
2.39.0

[PATCH 08/11] lib: sbi: Add system_suspend_allowed domain property

[Andrew Jones]

Only privileged domains should be allowed to suspend the entire
system. Give the root domain this property by default and allow
other domains to be given the property by specifying it in the
DT.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 docs/domain_support.md     | 5 +++++
 include/sbi/sbi_domain.h   | 2 ++
 lib/sbi/sbi_domain.c       | 4 ++++
 lib/utils/fdt/fdt_domain.c | 7 +++++++
 4 files changed, 18 insertions(+)

diff --git a/docs/domain_support.md b/docs/domain_support.md
index 4ccbcff87195..e2ff896bd550 100644
--- a/docs/domain_support.md
+++ b/docs/domain_support.md
@@ -52,6 +52,7 @@ has following details:
 * **next_mode** - Privilege mode of the next booting stage for this
   domain. This can be either S-mode or U-mode.
 * **system_reset_allowed** - Is domain allowed to reset the system?
+* **system_suspend_allowed** - Is domain allowed to suspend the system?
 
 The memory regions represented by **regions** in **struct sbi_domain** have
 following additional constraints to align with RISC-V PMP requirements:
@@ -91,6 +92,7 @@ following manner:
 * **next_mode** - Next booting stage mode in coldboot HART scratch space
   is the next mode for the ROOT domain
 * **system_reset_allowed** - The ROOT domain is allowed to reset the system
+* **system_suspend_allowed** - The ROOT domain is allowed to suspend the system
 
 Domain Effects
 --------------
@@ -195,6 +197,8 @@ The DT properties of a domain instance DT node are as follows:
   stage mode of coldboot HART** is used as default value.
 * **system-reset-allowed** (Optional) - A boolean flag representing
   whether the domain instance is allowed to do system reset.
+* **system-suspend-allowed** (Optional) - A boolean flag representing
+  whether the domain instance is allowed to do system suspend.
 
 ### Assigning HART To Domain Instance
 
@@ -260,6 +264,7 @@ be done:
                 next-addr = <0x0 0x80100000>;
                 next-mode = <0x0>;
                 system-reset-allowed;
+                system-suspend-allowed;
             };
 
             udomain: untrusted-domain {
diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
index bbb3effa2449..178221e3711d 100644
--- a/include/sbi/sbi_domain.h
+++ b/include/sbi/sbi_domain.h
@@ -120,6 +120,8 @@ struct sbi_domain {
 	unsigned long next_mode;
 	/** Is domain allowed to reset the system */
 	bool system_reset_allowed;
+	/** Is domain allowed to suspend the system */
+	bool system_suspend_allowed;
 };
 
 /** The root domain instance */
diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 60fda012d7b4..f2fe4da6d17f 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -34,6 +34,7 @@ struct sbi_domain root = {
 	.possible_harts = &root_hmask,
 	.regions = root_memregs,
 	.system_reset_allowed = true,
+	.system_suspend_allowed = true,
 };
 
 bool sbi_domain_is_assigned_hart(const struct sbi_domain *dom, u32 hartid)
@@ -394,6 +395,9 @@ void sbi_domain_dump(const struct sbi_domain *dom, const char *suffix)
 
 	sbi_printf("Domain%d SysReset    %s: %s\n",
 		   dom->index, suffix, (dom->system_reset_allowed) ? "yes" : "no");
+
+	sbi_printf("Domain%d SysSuspend  %s: %s\n",
+		   dom->index, suffix, (dom->system_suspend_allowed) ? "yes" : "no");
 }
 
 void sbi_domain_dump_all(const char *suffix)
diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
index 2b51a8ebe132..33653e2f555f 100644
--- a/lib/utils/fdt/fdt_domain.c
+++ b/lib/utils/fdt/fdt_domain.c
@@ -419,6 +419,13 @@ static int __fdt_parse_domain(void *fdt, int domain_offset, void *opaque)
 	else
 		dom->system_reset_allowed = false;
 
+	/* Read "system-suspend-allowed" DT property */
+	if (fdt_get_property(fdt, domain_offset,
+			     "system-suspend-allowed", NULL))
+		dom->system_suspend_allowed = true;
+	else
+		dom->system_suspend_allowed = false;
+
 	/* Find /cpus DT node */
 	cpus_offset = fdt_path_offset(fdt, "/cpus");
 	if (cpus_offset < 0)
-- 
2.39.0

[PATCH 09/11] lib: sbi: Implement system suspend

[Andrew Jones]

Fill the implementation of the system suspend ecall. A platform
implementation of the suspend callbacks is still required for this
to do anything.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 lib/sbi/sbi_system.c | 57 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/lib/sbi/sbi_system.c b/lib/sbi/sbi_system.c
index 5c123a6c9d8d..c18562cdcfab 100644
--- a/lib/sbi/sbi_system.c
+++ b/lib/sbi/sbi_system.c
@@ -116,5 +116,60 @@ bool sbi_system_suspend_supported(u32 sleep_type)
 
 int sbi_system_suspend(u32 sleep_type, ulong resume_addr, ulong opaque)
 {
-	return 0;
+	int ret = SBI_ENOTSUPP;
+	const struct sbi_domain *dom = sbi_domain_thishart_ptr();
+	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
+	void (*jump_warmboot)(void) = (void (*)(void))scratch->warmboot_addr;
+	unsigned int hartid = current_hartid();
+	unsigned long prev_mode;
+	unsigned long i;
+
+	if (!dom || !dom->system_suspend_allowed)
+		return SBI_EFAIL;
+
+	if (!suspend_dev || !suspend_dev->system_suspend)
+		return SBI_EFAIL;
+
+	if (!sbi_system_suspend_supported(sleep_type))
+		return SBI_ENOTSUPP;
+
+	prev_mode = (csr_read(CSR_MSTATUS) & MSTATUS_MPP) >> MSTATUS_MPP_SHIFT;
+	if (prev_mode != PRV_S && prev_mode != PRV_U)
+		return SBI_EFAIL;
+
+	sbi_hartmask_for_each_hart(i, &dom->assigned_harts) {
+		if (i == hartid)
+			continue;
+		if (__sbi_hsm_hart_get_state(i) != SBI_HSM_STATE_STOPPED)
+			return SBI_EFAIL;
+	}
+
+	if (!sbi_domain_check_addr(dom, resume_addr, prev_mode,
+				   SBI_DOMAIN_EXECUTE))
+		return SBI_EINVALID_ADDR;
+
+	if (!sbi_hsm_hart_change_state(scratch, SBI_HSM_STATE_STARTED,
+				       SBI_HSM_STATE_SUSPENDED))
+		return SBI_EFAIL;
+
+	/* Prepare for resume */
+	scratch->next_mode = prev_mode;
+	scratch->next_addr = resume_addr;
+	scratch->next_arg1 = opaque;
+
+	__sbi_hsm_suspend_non_ret_save(scratch);
+
+	/* Suspend */
+	ret = suspend_dev->system_suspend(sleep_type);
+	if (ret != SBI_OK) {
+		if (!sbi_hsm_hart_change_state(scratch, SBI_HSM_STATE_SUSPENDED,
+					       SBI_HSM_STATE_STARTED))
+			sbi_hart_hang();
+		return ret;
+	}
+
+	/* Resume */
+	jump_warmboot();
+
+	__builtin_unreachable();
 }
-- 
2.39.0

[PATCH 10/11] docs: Correct opensbi-domain property name

[Andrew Jones]

Replace the commas with dashes to correct the name.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 docs/domain_support.md | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/domain_support.md b/docs/domain_support.md
index e2ff896bd550..265d8d85786e 100644
--- a/docs/domain_support.md
+++ b/docs/domain_support.md
@@ -207,9 +207,9 @@ platform support can provide the HART to domain instance assignment using
 platform specific callback.
 
 The HART to domain instance assignment can be parsed from the device tree
-using optional DT property **opensbi,domain** in each CPU DT node. The
-value of DT property **opensbi,domain** is the DT phandle of the domain
-instance DT node. If **opensbi,domain** DT property is not specified then
+using optional DT property **opensbi-domain** in each CPU DT node. The
+value of DT property **opensbi-domain** is the DT phandle of the domain
+instance DT node. If **opensbi-domain** DT property is not specified then
 corresponding HART is assigned to **the ROOT domain**.
 
 ### Domain Configuration Only Accessible to OpenSBI
-- 
2.39.0

[PATCH 11/11] platform: generic: Add system suspend test

[Andrew Jones]

When the system-suspend-test property is present in the domain config
node as shown below, implement system suspend with a simple 5 second
delay followed by a WFI. This allows testing system suspend when the
low-level firmware doesn't support it.

  / {
    chosen {
      opensbi-domains {
          compatible = "opensbi,domain,config";
          system-suspend-test;
      };

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 docs/domain_support.md      |  4 ++++
 include/sbi/sbi_system.h    |  1 +
 lib/sbi/sbi_system.c        | 30 ++++++++++++++++++++++++++++++
 platform/generic/platform.c | 20 +++++++++++++++++++-
 4 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/docs/domain_support.md b/docs/domain_support.md
index 265d8d85786e..c13014a5f7cf 100644
--- a/docs/domain_support.md
+++ b/docs/domain_support.md
@@ -126,6 +126,9 @@ The DT properties of a domain configuration DT node are as follows:
 * **compatible** (Mandatory) - The compatible string of the domain
   configuration. This DT property should have value *"opensbi,domain,config"*
 
+* **system-suspend-test** (Optional) - When present, enable a system
+  suspend test implementation which simply waits five seconds and issues a WFI.
+
 ### Domain Memory Region Node
 
 The domain memory region DT node describes details of a memory region and
@@ -234,6 +237,7 @@ be done:
     chosen {
         opensbi-domains {
             compatible = "opensbi,domain,config";
+            system-suspend-test;
 
             tmem: tmem {
                 compatible = "opensbi,domain,memregion";
diff --git a/include/sbi/sbi_system.h b/include/sbi/sbi_system.h
index 65ea3d36d6cf..7d0867f276aa 100644
--- a/include/sbi/sbi_system.h
+++ b/include/sbi/sbi_system.h
@@ -57,6 +57,7 @@ struct sbi_system_suspend_device {
 
 const struct sbi_system_suspend_device *sbi_system_suspend_get_device(void);
 void sbi_system_suspend_set_device(struct sbi_system_suspend_device *dev);
+void sbi_system_suspend_test_enable(void);
 bool sbi_system_suspend_supported(u32 sleep_type);
 int sbi_system_suspend(u32 sleep_type, ulong resume_addr, ulong opaque);
 
diff --git a/lib/sbi/sbi_system.c b/lib/sbi/sbi_system.c
index c18562cdcfab..3242b21699cb 100644
--- a/lib/sbi/sbi_system.c
+++ b/lib/sbi/sbi_system.c
@@ -17,6 +17,7 @@
 #include <sbi/sbi_system.h>
 #include <sbi/sbi_ipi.h>
 #include <sbi/sbi_init.h>
+#include <sbi/sbi_timer.h>
 
 static SBI_LIST_HEAD(reset_devices_list);
 
@@ -108,6 +109,35 @@ void sbi_system_suspend_set_device(struct sbi_system_suspend_device *dev)
 	suspend_dev = dev;
 }
 
+static int sbi_system_suspend_test_check(u32 sleep_type)
+{
+	return sleep_type == SBI_SUSP_SLEEP_TYPE_SUSPEND;
+}
+
+static int sbi_system_suspend_test_suspend(u32 sleep_type)
+{
+	if (sleep_type != SBI_SUSP_SLEEP_TYPE_SUSPEND)
+		return SBI_EINVAL;
+
+	sbi_timer_mdelay(5000);
+
+	/* Wait for interrupt */
+	wfi();
+
+	return SBI_OK;
+}
+
+static struct sbi_system_suspend_device sbi_system_suspend_test = {
+	.name = "system-suspend-test",
+	.system_suspend_check = sbi_system_suspend_test_check,
+	.system_suspend = sbi_system_suspend_test_suspend,
+};
+
+void sbi_system_suspend_test_enable(void)
+{
+	sbi_system_suspend_set_device(&sbi_system_suspend_test);
+}
+
 bool sbi_system_suspend_supported(u32 sleep_type)
 {
 	return suspend_dev && suspend_dev->system_suspend_check &&
diff --git a/platform/generic/platform.c b/platform/generic/platform.c
index a34d3b019335..224cadc73a03 100644
--- a/platform/generic/platform.c
+++ b/platform/generic/platform.c
@@ -13,6 +13,7 @@
 #include <sbi/sbi_hartmask.h>
 #include <sbi/sbi_platform.h>
 #include <sbi/sbi_string.h>
+#include <sbi/sbi_system.h>
 #include <sbi_utils/fdt/fdt_domain.h>
 #include <sbi_utils/fdt/fdt_fixup.h>
 #include <sbi_utils/fdt/fdt_helper.h>
@@ -226,7 +227,24 @@ static int generic_extensions_init(struct sbi_hart_features *hfeatures)
 
 static int generic_domains_init(void)
 {
-	return fdt_domains_populate(fdt_get_address());
+	void *fdt = fdt_get_address();
+	int offset, ret;
+
+	ret = fdt_domains_populate(fdt);
+	if (ret < 0)
+		return ret;
+
+	offset = fdt_path_offset(fdt, "/chosen");
+
+	if (offset >= 0) {
+		offset = fdt_node_offset_by_compatible(fdt, offset,
+						       "opensbi,domain,config");
+		if (offset >= 0 &&
+		    fdt_get_property(fdt, offset, "system-suspend-test", NULL))
+			sbi_system_suspend_test_enable();
+	}
+
+	return 0;
 }
 
 static u64 generic_tlbr_flush_limit(void)
-- 
2.39.0

[PATCH 00/11] SBI system suspend (SUSP) extension

[Andrew Jones]

On Wed, Jan 18, 2023 at 03:42:23PM +0100, Andrew Jones wrote:
> This series implements the SBI system suspend support in OpenSBI
> as-per the draft proposal for SBI system suspend which can be found at
> https://github.com/jones-drew/riscv-sbi-doc/commit/d9e43e9a938fc3eb510e023c3f352462876f7785
> 
> The first half of the series are a few cleanups and fixes for HSM,
> as system suspend makes use of some of its code and patterns. The
> second half implements support for the extension and includes a patch
> which allows testing to be enabled, even when low-level firmware does
> not provide support. Regarding the test mode, it's currently enabled
> by adding a 'system-suspend-test' DT property, but Anup has suggested
> that FW_OPTION could also be used. I'm all ears for what people think
> the best way to control test functionality in OpenSBI is and am happy
> to switch it. To test with Linux the kernel must include the patch at
> https://github.com/jones-drew/linux/commits/riscv/sbi-susp-rfc and
> be built with CONFIG_SUSPEND enabled.
> 
> The patches are based on commit da5594bf853d ("platform: generic:
> allwinner: Fix PLIC array bounds") and can also be found at
> https://github.com/jones-drew/opensbi/commits/susp-v1
> 
> v2:
  ^ This should be v1, since I started at RFC (v0).

>  - Rebased on latest master
>  - Added a print line to sbi_boot_print_general() [Anup]
>  - Picked up Anup's r-b's
> 
> Andrew Jones (11):
>   lib: sbi_hsm: Factor out invalid state detection
>   lib: sbi_hsm: Don't try to restore state on failed change
>   lib: sbi_hsm: Ensure errors are consistent with spec
>   lib: sbi_hsm: Move misplaced comment
>   lib: sbi_hsm: Remove unnecessary include
>   lib: sbi_hsm: Export some functions
>   lib: sbi: Add system suspend skeleton
>   lib: sbi: Add system_suspend_allowed domain property
>   lib: sbi: Implement system suspend
>   docs: Correct opensbi-domain property name
>   platform: generic: Add system suspend test
> 
>  docs/domain_support.md            |  15 +++-
>  include/sbi/sbi_domain.h          |   2 +
>  include/sbi/sbi_ecall_interface.h |   8 +++
>  include/sbi/sbi_hsm.h             |   4 ++
>  include/sbi/sbi_system.h          |  18 +++++
>  lib/sbi/Kconfig                   |   4 ++
>  lib/sbi/objects.mk                |   3 +
>  lib/sbi/sbi_domain.c              |   4 ++
>  lib/sbi/sbi_ecall_hsm.c           |   4 +-
>  lib/sbi/sbi_ecall_susp.c          |  48 +++++++++++++
>  lib/sbi/sbi_hsm.c                 |  92 +++++++++++--------------
>  lib/sbi/sbi_init.c                |   4 ++
>  lib/sbi/sbi_system.c              | 111 ++++++++++++++++++++++++++++++
>  lib/utils/fdt/fdt_domain.c        |   7 ++
>  platform/generic/platform.c       |  20 +++++-
>  15 files changed, 287 insertions(+), 57 deletions(-)
>  create mode 100644 lib/sbi/sbi_ecall_susp.c
> 
> -- 
> 2.39.0
>