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

From: Conor Dooley <conor@kernel.org>
To: Andrew Jones <ajones@ventanamicro.com>
Cc: linux-riscv@lists.infradead.org, paul.walmsley@sifive.com,
	palmer@dabbelt.com, aou@eecs.berkeley.edu,
	leyfoon.tan@starfivetech.com, jeeheng.sia@starfivetech.com,
	conor.dooley@microchip.com, apatel@ventanamicro.com,
	samuel.holland@sifive.com, songshuaishuai@tinylab.org
Subject: Re: [PATCH v3 1/1] riscv: sbi: Introduce system suspend support
Date: Wed, 6 Dec 2023 12:23:16 +0000	[thread overview]
Message-ID: <20231206-prone-detention-8d06fe96f34c@spud> (raw)
In-Reply-To: <20231206110807.35882-4-ajones@ventanamicro.com>

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

On Wed, Dec 06, 2023 at 12:08:09PM +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>
> Tested-by: Samuel Holland <samuel.holland@sifive.com>

> +#ifdef CONFIG_RISCV_SBI
> +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_SYSTEM_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_TO_RAM, 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_version >= sbi_mk_version(2, 0) &&
> +	    sbi_probe_extension(SBI_EXT_SUSP) > 0) {
> +		pr_info("SBI SUSP extension detected\n");

Is there any value in probing and printing this stuff out if
the kernel is not actually built with support for suspend?
I can see either that or moving the IS_ENABLED() check so that
it controls whether we even probe for SUSP being valid so
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> +		if (IS_ENABLED(CONFIG_SUSPEND))
> +			suspend_set_ops(&sbi_system_suspend_ops);
> +	}
> +
> +	return 0;
> +}
> +
> +arch_initcall(sbi_system_suspend_init);
> +#endif /* CONFIG_RISCV_SBI */
> -- 
> 2.43.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