Re: [RFC PATCH 1/1] riscv: sbi: Introduce system suspend support

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4668 bytes --]

Hey Drew!

On Fri, Jan 06, 2023 at 12:32:16PM +0100, Andrew Jones wrote:
> When the SUSP SBI extension is present it implies that the standard
> "suspend to RAM" type is available. Wire it up to the generic
> platform suspend support, also applying the already present support
> for non-retentive CPU suspend. When the kernel is built with
> CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend.
> Resumption will occur when a platform-specific wake-up event arrives.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

First things first, anything SBI depends on !m-mode, so you've gotta add
some sort of gating unfortunately around those ECALLs. But I figure you
may have already seen the build failures on patchwork for nommu?

Also, when there's an actual spec would you mind doing a Link: spec.pdf?

> ---
>  arch/riscv/Kconfig           |  5 ++++-
>  arch/riscv/include/asm/sbi.h |  9 ++++++++
>  arch/riscv/kernel/suspend.c  | 41 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index e2b656043abf..a53d94c1953e 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -52,7 +52,7 @@ config RISCV
>  	select CLONE_BACKWARDS
>  	select CLINT_TIMER if !MMU
>  	select COMMON_CLK
> -	select CPU_PM if CPU_IDLE
> +	select CPU_PM if (SUSPEND || CPU_IDLE)
>  	select EDAC_SUPPORT
>  	select GENERIC_ARCH_TOPOLOGY
>  	select GENERIC_ATOMIC64 if !64BIT
> @@ -686,6 +686,9 @@ config PORTABLE
>  	select OF
>  	select MMU
>  
> +config ARCH_SUSPEND_POSSIBLE
> +	def_bool y

Since the content you're adding depends on having an SBI extention,
does this need to be s/y/RISCV_SBI/?

>  menu "Power management options"
>  
>  source "kernel/power/Kconfig"
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 4ca7fbacff42..9834ba4ce3e4 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -29,6 +29,7 @@ enum sbi_ext_id {
>  	SBI_EXT_RFENCE = 0x52464E43,
>  	SBI_EXT_HSM = 0x48534D,
>  	SBI_EXT_SRST = 0x53525354,
> +	SBI_EXT_SUSP = 0x53555350,
>  	SBI_EXT_PMU = 0x504D55,
>  
>  	/* Experimentals extensions must lie within this range */
> @@ -113,6 +114,14 @@ enum sbi_srst_reset_reason {
>  	SBI_SRST_RESET_REASON_SYS_FAILURE,
>  };
>  
> +enum sbi_ext_susp_fid {
> +	SBI_EXT_SUSP_SUSPEND = 0,
> +};
> +
> +enum sbi_ext_susp_sleep_type {
> +	SBI_SUSP_SLEEP_TYPE_SUSPEND = 0,
> +};
> +
>  enum sbi_ext_pmu_fid {
>  	SBI_EXT_PMU_NUM_COUNTERS = 0,
>  	SBI_EXT_PMU_COUNTER_GET_INFO,
> diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
> index 9ba24fb8cc93..bc26e9ae4782 100644
> --- a/arch/riscv/kernel/suspend.c
> +++ b/arch/riscv/kernel/suspend.c
> @@ -4,8 +4,12 @@
>   * Copyright (c) 2022 Ventana Micro Systems Inc.
>   */
>  
> +#define pr_fmt(fmt) "suspend: " fmt
> +
>  #include <linux/ftrace.h>
> +#include <linux/suspend.h>
>  #include <asm/csr.h>
> +#include <asm/sbi.h>
>  #include <asm/suspend.h>
>  
>  static void suspend_save_csrs(struct suspend_context *context)
> @@ -85,3 +89,40 @@ int cpu_suspend(unsigned long arg,
>  
>  	return rc;
>  }

And then from here down needs to be #ifdef RISCV_SBI?

Anyways, probably stating the obvious on an RFC, but ¯\_(ツ)_/¯

Thanks,
Conor.

> +
> +static int sbi_system_suspend(unsigned long sleep_type,
> +			      unsigned long resume_addr,
> +			      unsigned long opaque)
> +{
> +	struct sbiret ret;
> +
> +	ret = sbi_ecall(SBI_EXT_SUSP, SBI_EXT_SUSP_SUSPEND,
> +			sleep_type, resume_addr, opaque, 0, 0, 0);
> +	if (ret.error)
> +		return sbi_err_map_linux_errno(ret.error);
> +
> +	return ret.value;
> +}
> +
> +static int sbi_system_suspend_enter(suspend_state_t state)
> +{
> +	return cpu_suspend(SBI_SUSP_SLEEP_TYPE_SUSPEND, sbi_system_suspend);
> +}
> +
> +static const struct platform_suspend_ops sbi_system_suspend_ops = {
> +	.valid = suspend_valid_only_mem,
> +	.enter = sbi_system_suspend_enter,
> +};
> +
> +static int __init sbi_system_suspend_init(void)
> +{
> +	if (!sbi_spec_is_0_1() && sbi_probe_extension(SBI_EXT_SUSP) > 0) {
> +		pr_info("SBI SUSP extension detected\n");
> +		if (IS_ENABLED(CONFIG_SUSPEND))
> +			suspend_set_ops(&sbi_system_suspend_ops);
> +	}
> +
> +	return 0;
> +}
> +
> +arch_initcall(sbi_system_suspend_init);
> -- 
> 2.39.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv