SBI system suspend (SUSP) extension
cover SBI system suspend (SUSP) extension
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
Signature · 1 line
2.39.0
PATCH 1/11 lib: sbi_hsm: Factor out invalid state detection
Remove some redundant code by creating an invalid state detection macro.
No functional change intended.
--- lib/sbi/sbi_hsm.c | 65 +++++++++++++++++------------------------------ 1 file changed, 24 insertions(+), 41 deletions(-)
lib/sbi/sbi_hsm.c +24 -41
@@ -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; }
Signature · 1 line
2.39.0
PATCH 2/11 lib: sbi_hsm: Don't try to restore state on failed change
In reply to Andrew JonesWhen a state change fails there's no need to restore the original state as it remains the same.
--- lib/sbi/sbi_hsm.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
lib/sbi/sbi_hsm.c +2 -5
@@ -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.
Signature · 1 line
2.39.0
PATCH 3/11 lib: sbi_hsm: Ensure errors are consistent with spec
In reply to Andrew JonesHSM 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.
--- lib/sbi/sbi_hsm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
lib/sbi/sbi_hsm.c +3 -3
@@ -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;
Signature · 1 line
2.39.0
PATCH 4/11 lib: sbi_hsm: Move misplaced comment
In reply to Andrew JonesWhile 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.
--- lib/sbi/sbi_hsm.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
lib/sbi/sbi_hsm.c +4 -2
@@ -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,
Signature · 1 line
2.39.0
PATCH 5/11 lib: sbi_hsm: Remove unnecessary include
In reply to Andrew JonesAlso remove a superfluous semicolon and add a blank line.
--- lib/sbi/sbi_ecall_hsm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
lib/sbi/sbi_ecall_hsm.c +2 -2
@@ -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 ;
Signature · 1 line
2.39.0
PATCH 6/11 lib: sbi_hsm: Export some functions
In reply to Andrew JonesA coming patch can make use of a few internal hsm functions if we export them.
--- include/sbi/sbi_hsm.h | 4 ++++ lib/sbi/sbi_hsm.c | 12 ++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-)
include/sbi/sbi_hsm.h +4 -0
@@ -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
lib/sbi/sbi_hsm.c +10 -2
@@ -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);
Signature · 1 line
2.39.0
PATCH 7/11 lib: sbi: Add system suspend skeleton
In reply to Andrew JonesAdd the SUSP extension probe and ecall support, but for now the system suspend function is just a stub.
--- 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
include/sbi/sbi_ecall_interface.h +8 -0
@@ -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
include/sbi/sbi_system.h +17 -0
@@ -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
lib/sbi/Kconfig +4 -0
@@ -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
lib/sbi/objects.mk +3 -0
@@ -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
lib/sbi/sbi_ecall_susp.c new file +48 -0
48 lines not on this page · raw message
lib/sbi/sbi_init.c +4 -0
@@ -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);
lib/sbi/sbi_system.c +26 -0
@@ -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 ; +}
Signature · 1 line
2.39.0
PATCH 8/11 lib: sbi: Add system_suspend_allowed domain property
In reply to Andrew JonesOnly 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.
--- 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(+)
docs/domain_support.md +5 -0
@@ -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 {
include/sbi/sbi_domain.h +2 -0
@@ -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 */
lib/sbi/sbi_domain.c +4 -0
@@ -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)
lib/utils/fdt/fdt_domain.c +7 -0
@@ -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 )
Signature · 1 line
2.39.0
PATCH 9/11 lib: sbi: Implement system suspend
In reply to Andrew JonesFill the implementation of the system suspend ecall. A platform implementation of the suspend callbacks is still required for this to do anything.
--- lib/sbi/sbi_system.c | 57 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-)
lib/sbi/sbi_system.c +56 -1
@@ -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 (); }
Signature · 1 line
2.39.0
PATCH 10/11 docs: Correct opensbi-domain property name
In reply to Andrew JonesReplace the commas with dashes to correct the name.
--- docs/domain_support.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
docs/domain_support.md +3 -3
@@ -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
Signature · 1 line
2.39.0
PATCH 11/11 platform: generic: Add system suspend test
In reply to Andrew JonesWhen 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;
};--- 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(-)
docs/domain_support.md +4 -0
@@ -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";
include/sbi/sbi_system.h +1 -0
@@ -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);
lib/sbi/sbi_system.c +30 -0
@@ -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 &&
platform/generic/platform.c +19 -1
@@ -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 )
Signature · 1 line
2.39.0
cover SBI system suspend (SUSP) extension
In reply to Andrew JonesQuoted 16 lines · 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/d9e43e9a938fc3eb510e023c3f352462876f7785The 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_OPTIONcould 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 withCONFIG_SUSPENDenabled.
Quoted 5 lines
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-v1v2:
^ This should be v1, since I started at RFC (v0).
Quoted 38 lines
- Rebased on latest master
- Added a print line tosbi_boot_print_general()[Anup]
- Picked up Anup's r-b'sAndrew 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: Addsystem_suspend_alloweddomain property
lib: sbi: Implement system suspend
docs: Correct opensbi-domain property name
platform: generic: Add system suspend testdocs/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.cSignature · 1 line
2.39.0