[PATCH 0/2] firmware: meson: sm: make power-off work on S4

[PATCH 0/2] firmware: meson: sm: make power-off work on S4

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

On S4 the firmware implements PSCI SYSTEM_OFF as DDR self-refresh, and
the power key then resumes into a kernel that is gone, so the board
hangs until it is unplugged. The vendor kernel sets a firmware shutdown
flag first and takes the secondary CPUs down through PSCI; do the same,
keyed on a new amlogic,meson-s4-sm compatible.

Tested: poweroff followed by the remote's power key cold-boots the box.

Tool use (Documentation/process/generated-content.rst):
This work was done with the help of an AI coding assistant, Claude
(Anthropic), used through Claude Code over many sessions. It did a large
part of the hands-on work: it read the vendor kernel and the firmware
interfaces, probed registers on the board, drafted the code and the
changelogs, and ran the builds, checkpatch, dt_binding_check and the
tests on the hardware. I directed the work, reviewed every change and
the final patches, and take responsibility for them. There is no single
prompt to quote; the sessions were of the form "find out why X fails on
this board, fix it, test it on the box".

Tested on an Amediatech X98Q TV box (Amlogic S905W2, S4 family, 2 GiB),
vendor U-Boot and BL31/OP-TEE, Linux 7.3 plus this work.

Christian Stefan Kövér-Draxl (2):
  dt-bindings: firmware: amlogic,meson-gxbb-sm: add amlogic,meson-s4-sm
  firmware: meson: sm: make power-off work on S4

 .../firmware/amlogic,meson-gxbb-sm.yaml       |  4 +-
 drivers/firmware/meson/meson_sm.c             | 46 +++++++++++++++++++
 include/linux/firmware/meson/meson_sm.h       |  1 +
 3 files changed, 50 insertions(+), 1 deletion(-)


base-commit: 7be4bc52fd9e0e55dc9a2e0232b89427d2c4b614
-- 
2.56.0


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

[PATCH 1/2] dt-bindings: firmware: amlogic,meson-gxbb-sm: add amlogic,meson-s4-sm

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

The S4 secure monitor implements the GXBB interface, but its PSCI
SYSTEM_OFF needs an extra firmware call first (a shutdown flag), so the
driver has to tell it apart. Add an S4 compatible with the GXBB one as
fallback.

Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
---
 .../devicetree/bindings/firmware/amlogic,meson-gxbb-sm.yaml   | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/firmware/amlogic,meson-gxbb-sm.yaml b/Documentation/devicetree/bindings/firmware/amlogic,meson-gxbb-sm.yaml
index 8f50e698760e..6ff2ddfcaf54 100644
--- a/Documentation/devicetree/bindings/firmware/amlogic,meson-gxbb-sm.yaml
+++ b/Documentation/devicetree/bindings/firmware/amlogic,meson-gxbb-sm.yaml
@@ -18,7 +18,9 @@ properties:
     oneOf:
       - const: amlogic,meson-gxbb-sm
       - items:
-          - const: amlogic,meson-gx-sm
+          - enum:
+              - amlogic,meson-gx-sm
+              - amlogic,meson-s4-sm
           - const: amlogic,meson-gxbb-sm
 
   power-controller:
-- 
2.56.0


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

[PATCH 2/2] firmware: meson: sm: make power-off work on S4

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

On S4 the firmware implements PSCI SYSTEM_OFF as DDR self-refresh. A
press of the power key afterwards jumps back to the kernel resume
address, where nothing is waiting, and the board hangs until it is
unplugged.

The vendor kernel avoids this in two steps, both needed:

 - set the firmware shutdown flag (SMC 0x82000042, argument 1) right
   before SYSTEM_OFF, which makes the firmware cold-boot on the power
   key instead of resuming;
 - take the secondary CPUs down through PSCI CPU_OFF first. With them
   only parked in the kernel, the firmware wake-up path hangs as well.

Register a power-off prepare handler that stops the secondary CPUs and
a high priority power-off handler that sets the flag, for the
amlogic,meson-s4-sm compatible only.

Tested on an S905W2 board: poweroff followed by the power key
cold-boots the board.

Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
---
 drivers/firmware/meson/meson_sm.c       | 46 +++++++++++++++++++++++++
 include/linux/firmware/meson/meson_sm.h |  1 +
 2 files changed, 47 insertions(+)

diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
index ab9751a59b55..5410386603c7 100644
--- a/drivers/firmware/meson/meson_sm.c
+++ b/drivers/firmware/meson/meson_sm.c
@@ -10,6 +10,7 @@
 
 #include <linux/arm-smccc.h>
 #include <linux/bug.h>
+#include <linux/cpu.h>
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -17,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/printk.h>
 #include <linux/property.h>
+#include <linux/reboot.h>
 #include <linux/types.h>
 #include <linux/sizes.h>
  #include <linux/slab.h>
@@ -48,6 +50,7 @@ static const struct meson_sm_chip gxbb_chip = {
 		CMD(SM_THERMAL_CALIB_READ,	0x82000047),
 		CMD(SM_A1_PWRC_SET,		0x82000093),
 		CMD(SM_A1_PWRC_GET,		0x82000095),
+		CMD(SM_SET_POWEROFF_FLAG,	0x82000042),
 		{ /* sentinel */ },
 	},
 };
@@ -303,6 +306,45 @@ static struct attribute *meson_sm_sysfs_attrs[] = {
 };
 ATTRIBUTE_GROUPS(meson_sm_sysfs);
 
+/*
+ * The S4 firmware implements PSCI SYSTEM_OFF as DDR self-refresh: on the
+ * power key it jumps back to the kernel instead of booting, which hangs the
+ * board. Setting the shutdown flag first makes it cold-boot instead, and the
+ * secondary CPUs must have been taken down through PSCI CPU_OFF beforehand,
+ * otherwise the wake-up path hangs as well.
+ */
+static int meson_sm_power_off_prepare(struct sys_off_data *data)
+{
+	if (freeze_secondary_cpus(0))
+		pr_warn("meson-sm: failed to stop secondary CPUs\n");
+
+	return NOTIFY_DONE;
+}
+
+static int meson_sm_power_off(struct sys_off_data *data)
+{
+	meson_sm_call(data->cb_data, SM_SET_POWEROFF_FLAG, NULL, 1, 0, 0, 0, 0);
+
+	return NOTIFY_DONE;
+}
+
+static int meson_sm_register_power_off(struct device *dev,
+				       struct meson_sm_firmware *fw)
+{
+	int ret;
+
+	ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE,
+					    SYS_OFF_PRIO_DEFAULT,
+					    meson_sm_power_off_prepare, NULL);
+	if (ret)
+		return ret;
+
+	/* Run before the PSCI handler, which never returns */
+	return devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
+					     SYS_OFF_PRIO_HIGH,
+					     meson_sm_power_off, fw);
+}
+
 static const struct of_device_id meson_sm_ids[] = {
 	{ .compatible = "amlogic,meson-gxbb-sm", .data = &gxbb_chip },
 	{ /* sentinel */ },
@@ -343,6 +385,10 @@ static int __init meson_sm_probe(struct platform_device *pdev)
 	if (devm_of_platform_populate(dev))
 		goto unmap_out_base;
 
+	if (of_device_is_compatible(dev->of_node, "amlogic,meson-s4-sm") &&
+	    meson_sm_register_power_off(dev, fw))
+		dev_warn(dev, "failed to register power-off handlers\n");
+
 	pr_info("secure-monitor enabled\n");
 
 	return 0;
diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h
index 3ebc2bd9a976..3f2fa160e32f 100644
--- a/include/linux/firmware/meson/meson_sm.h
+++ b/include/linux/firmware/meson/meson_sm.h
@@ -15,6 +15,7 @@ enum {
 	SM_THERMAL_CALIB_READ,
 	SM_A1_PWRC_SET,
 	SM_A1_PWRC_GET,
+	SM_SET_POWEROFF_FLAG,
 };
 
 struct meson_sm_firmware;
-- 
2.56.0


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

Re: [PATCH 2/2] firmware: meson: sm: make power-off work on S4

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] firmware: meson: sm: broken CPU offlining via freeze_secondary_cpus()
- [Medium] firmware: meson: sm: build and link failure calling freeze_secondary_cpus()
- [Medium] firmware: meson_sm: missing cleanup on error in meson_sm_register_power_off()
--

commit 520b94ab82581e32dd9c97ba0f810c53f3ecadaf
Author: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

firmware: meson: sm: make power-off work on S4

This patch adds power-off prepare and power-off handlers for the Amlogic S4
secure monitor to offline secondary processors and set the firmware shutdown
flag prior to system power-off.

> diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
> index ab9751a59b552..5410386603c70 100644
> --- a/drivers/firmware/meson/meson_sm.c
> +++ b/drivers/firmware/meson/meson_sm.c

[ ... ]

> @@ -303,6 +306,45 @@ static struct attribute *meson_sm_sysfs_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(meson_sm_sysfs);
>  
> +/*
> + * The S4 firmware implements PSCI SYSTEM_OFF as DDR self-refresh: on the
> + * power key it jumps back to the kernel instead of booting, which hangs the
> + * board. Setting the shutdown flag first makes it cold-boot instead, and the
> + * secondary CPUs must have been taken down through PSCI CPU_OFF beforehand,
> + * otherwise the wake-up path hangs as well.
> + */
> +static int meson_sm_power_off_prepare(struct sys_off_data *data)
> +{
> +	if (freeze_secondary_cpus(0))
> +		pr_warn("meson-sm: failed to stop secondary CPUs\n");

[Severity: Medium]
Does calling freeze_secondary_cpus(0) in meson_sm_power_off_prepare() in
drivers/firmware/meson/meson_sm.c break the build when this driver is built
as a module or when sleep support is disabled?

In drivers/firmware/meson/Kconfig, the driver is tristate and does not
depend on sleep or symmetric multiprocessing support:

drivers/firmware/meson/Kconfig:
	config MESON_SM
		tristate "Amlogic Secure Monitor driver"
		depends on ARCH_MESON || COMPILE_TEST
		default ARCH_MESON
		depends on ARM64_4K_PAGES

However, include/linux/cpu.h only declares freeze_secondary_cpus() under
the sleep configuration guard and provides no stub in the else branch:

include/linux/cpu.h:
	#ifdef CONFIG_PM_SLEEP_SMP
	extern int freeze_secondary_cpus(int primary);
	extern void thaw_secondary_cpus(void);
	...
	#else /* !CONFIG_PM_SLEEP_SMP */
	static inline void thaw_secondary_cpus(void) {}
	static inline int suspend_disable_secondary_cpus(void) { return 0; }
	static inline void suspend_enable_secondary_cpus(void) { }
	#endif /* !CONFIG_PM_SLEEP_SMP */

In addition, freeze_secondary_cpus() in kernel/cpu.c is not exported for
loadable modules:

kernel/cpu.c:
	#ifdef CONFIG_PM_SLEEP_SMP
	static cpumask_var_t frozen_cpus;

	int freeze_secondary_cpus(int primary)

When sleep support is disabled, compiling drivers/firmware/meson/meson_sm.c
fails with an implicit function declaration error. When the driver is built
as a module (for example in an arm64 allmodconfig build), linking fails at
modpost with an undefined symbol error for freeze_secondary_cpus().

> +
> +	return NOTIFY_DONE;
> +}

[Severity: High]
Can calling the suspend helper freeze_secondary_cpus(0) from
meson_sm_power_off_prepare() during kernel_power_off() ->
do_kernel_power_off_prepare() fail to offline secondary processors after a
prior wakeup event?

Inside kernel/cpu.c:freeze_secondary_cpus(), the loop checks
pm_wakeup_pending() before taking down each processor and aborts if a wakeup
is pending:

kernel/cpu.c:freeze_secondary_cpus() {
	...
		if (pm_wakeup_pending()) {
			pr_info("Wakeup pending. Abort CPU freeze\n");
			error = -EBUSY;
			break;
		}

		trace_suspend_resume(TPS("CPU_OFF"), cpu, true);
		error = _cpu_down(cpu, 1, CPUHP_OFFLINE);
	...
}

In drivers/base/power/wakeup.c:pm_wakeup_pending(), the check returns true
whenever pm_abort_suspend is positive:

drivers/base/power/wakeup.c:pm_wakeup_pending() {
	...
	return ret || atomic_read(&pm_abort_suspend) > 0;
}

void pm_system_wakeup(void)
{
	atomic_inc(&pm_abort_suspend);
	s2idle_wake();
}

Because pm_abort_suspend is incremented by pm_system_wakeup() during a
suspend or s2idle wakeup event and is only cleared by pm_wakeup_clear(0) at
the start of a subsequent suspend or hibernate cycle (in pm_sleep_fs_sync()
and freeze_processes()), it remains positive after resume and is never
cleared along the kernel_power_off() path.

Furthermore, freeze_secondary_cpus(0) passes tasks_frozen = 1 to
_cpu_down(), which sets cpuhp_tasks_frozen during shutdown even though
tasks are not frozen.

Would powering off after any suspend/resume cycle or wakeup event cause
freeze_secondary_cpus(0) to abort with -EBUSY before offlining secondary
processors, leaving them parked in the kernel and reintroducing the
firmware wake-up hang?

> +
> +static int meson_sm_power_off(struct sys_off_data *data)
> +{
> +	meson_sm_call(data->cb_data, SM_SET_POWEROFF_FLAG, NULL, 1, 0, 0, 0, 0);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int meson_sm_register_power_off(struct device *dev,
> +				       struct meson_sm_firmware *fw)
> +{
> +	int ret;
> +
> +	ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE,
> +					    SYS_OFF_PRIO_DEFAULT,
> +					    meson_sm_power_off_prepare, NULL);
> +	if (ret)
> +		return ret;
> +
> +	/* Run before the PSCI handler, which never returns */
> +	return devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
> +					     SYS_OFF_PRIO_HIGH,
> +					     meson_sm_power_off, fw);
> +}
> +
>  static const struct of_device_id meson_sm_ids[] = {
>  	{ .compatible = "amlogic,meson-gxbb-sm", .data = &gxbb_chip },
>  	{ /* sentinel */ },
> @@ -343,6 +385,10 @@ static int __init meson_sm_probe(struct platform_device *pdev)
>  	if (devm_of_platform_populate(dev))
>  		goto unmap_out_base;
>  
> +	if (of_device_is_compatible(dev->of_node, "amlogic,meson-s4-sm") &&
> +	    meson_sm_register_power_off(dev, fw))
> +		dev_warn(dev, "failed to register power-off handlers\n");

[Severity: Medium]
If the second devm_register_sys_off_handler() call in
meson_sm_register_power_off() fails (for example, if registering at high
priority via register_sys_off_handler() ->
atomic_notifier_chain_register_unique_prio() returns -EBUSY or -ENOMEM),
meson_sm_probe() logs a warning and still returns 0.

Because meson_sm_probe() succeeds, devres cleanup does not run, leaving
meson_sm_power_off_prepare() registered on power_off_prep_handler_list
without its companion meson_sm_power_off() handler.

Should meson_sm_register_power_off() or meson_sm_probe() clean up the
prepare handler when registering the power-off handler fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025192144.208954-1-christian.koever-draxl@student.uibk.ac.at?part=2

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