[PATCH] power: reset: th1520-aon: send RPCs from sleepable handlers

[PATCH] power: reset: th1520-aon: send RPCs from sleepable handlers

[Han Gao]

The AON RPC takes a mutex and waits for mailbox interrupts. The final
sys-off handlers run on atomic notifier chains, where this can sleep
with interrupts disabled.

Send the requests from the preparation handlers before syscore shutdown.
Retain a final power-off handler to advertise capability and report
failure without issuing another RPC. Let other providers handle
fallback.

Report RPC errors and allow one second after a successful return for the
asynchronous operation to take effect. This is a software fallback
timeout, not a hardware timing requirement.

Fixes: 2d81a24a74e5 ("driver: reset: th1520-aon: add driver for poweroff/reboot via AON FW")
Signed-off-by: Han Gao <gaohan@iscas.ac.cn>
---
 drivers/power/reset/th1520-aon-reboot.c | 49 +++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/power/reset/th1520-aon-reboot.c b/drivers/power/reset/th1520-aon-reboot.c
index ec249667a0ffd74829357d57b9e63c3524ea3759..ace70491834dc9a63ed543427e40e4cbc0a9388f 100644
--- a/drivers/power/reset/th1520-aon-reboot.c
+++ b/drivers/power/reset/th1520-aon-reboot.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/auxiliary_bus.h>
+#include <linux/delay.h>
 #include <linux/firmware/thead/thead,th1520-aon.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
@@ -14,37 +15,46 @@
 #include <linux/slab.h>
 
 #define TH1520_AON_REBOOT_PRIORITY 200
+/* Allow an accepted asynchronous command to take effect before fallback. */
+#define TH1520_AON_REBOOT_TIMEOUT_MS 1000
 
 struct th1520_aon_msg_empty_body {
 	struct th1520_aon_rpc_msg_hdr hdr;
 	u16 reserved[12];
 } __packed __aligned(1);
 
-static int th1520_aon_pwroff_handler(struct sys_off_data *data)
+static int th1520_aon_reboot_request(struct sys_off_data *data, u8 func)
 {
 	struct th1520_aon_chan *aon_chan = data->cb_data;
 	struct th1520_aon_msg_empty_body msg = {};
+	int ret;
 
 	msg.hdr.svc = TH1520_AON_RPC_SVC_WDG;
-	msg.hdr.func = TH1520_AON_WDG_FUNC_POWER_OFF;
+	msg.hdr.func = func;
 	msg.hdr.size = TH1520_AON_RPC_MSG_NUM;
 
-	th1520_aon_call_rpc(aon_chan, &msg);
+	ret = th1520_aon_call_rpc(aon_chan, &msg);
+	if (ret)
+		dev_err(data->dev, "AON WDG command %u failed: %d\n", func, ret);
+	else
+		msleep(TH1520_AON_REBOOT_TIMEOUT_MS);
 
 	return NOTIFY_DONE;
 }
 
-static int th1520_aon_restart_handler(struct sys_off_data *data)
+static int th1520_aon_pwroff_handler(struct sys_off_data *data)
 {
-	struct th1520_aon_chan *aon_chan = data->cb_data;
-	struct th1520_aon_msg_empty_body msg = {};
-
-	msg.hdr.svc = TH1520_AON_RPC_SVC_WDG;
-	msg.hdr.func = TH1520_AON_WDG_FUNC_RESTART;
-	msg.hdr.size = TH1520_AON_RPC_MSG_NUM;
+	return th1520_aon_reboot_request(data, TH1520_AON_WDG_FUNC_POWER_OFF);
+}
 
-	th1520_aon_call_rpc(aon_chan, &msg);
+static int th1520_aon_restart_handler(struct sys_off_data *data)
+{
+	return th1520_aon_reboot_request(data, TH1520_AON_WDG_FUNC_RESTART);
+}
 
+static int th1520_aon_pwroff_failed(struct sys_off_data *data)
+{
+	dev_err(data->dev, "AON did not power off the system\n");
 	return NOTIFY_DONE;
 }
 
@@ -54,8 +64,11 @@ static int th1520_aon_reboot_probe(struct auxiliary_device *adev,
 	struct device *dev = &adev->dev;
 	int ret;
 
-	/* Expect struct th1520_aon_chan to be passed via platform_data */
-	ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
+	/*
+	 * RPC takes a mutex and waits for mailbox interrupts. Send after device
+	 * shutdown, but before syscore shutdown and the final atomic callbacks.
+	 */
+	ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE,
 					    TH1520_AON_REBOOT_PRIORITY,
 					    th1520_aon_pwroff_handler,
 					    adev->dev.platform_data);
@@ -65,7 +78,7 @@ static int th1520_aon_reboot_probe(struct auxiliary_device *adev,
 		return ret;
 	}
 
-	ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART,
+	ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART_PREPARE,
 					    TH1520_AON_REBOOT_PRIORITY,
 					    th1520_aon_restart_handler,
 					    adev->dev.platform_data);
@@ -75,7 +88,13 @@ static int th1520_aon_reboot_probe(struct auxiliary_device *adev,
 		return ret;
 	}
 
-	return 0;
+	/*
+	 * Advertise power-off capability to kernel_can_power_off(). This final
+	 * callback must not send RPCs; let other providers try if AON failed.
+	 */
+	return devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
+					     TH1520_AON_REBOOT_PRIORITY,
+					     th1520_aon_pwroff_failed, dev_get_platdata(dev));
 }
 
 static const struct auxiliary_device_id th1520_aon_reboot_id_table[] = {

---
base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
change-id: 20260916-fix-xuantieaon-1ea016a22526

Best regards,
-- 
Han Gao <gaohan@iscas.ac.cn>

Re: [PATCH] power: reset: th1520-aon: send RPCs from sleepable handlers

[Icenowy Zheng]

在 2026-09-16三的 14:55 +0800，Han Gao写道：
> The AON RPC takes a mutex and waits for mailbox interrupts. The final
> sys-off handlers run on atomic notifier chains, where this can sleep
> with interrupts disabled.
> 
> Send the requests from the preparation handlers before syscore
> shutdown.
> Retain a final power-off handler to advertise capability and report
> failure without issuing another RPC. Let other providers handle
> fallback.
> 
> Report RPC errors and allow one second after a successful return for
> the
> asynchronous operation to take effect. This is a software fallback
> timeout, not a hardware timing requirement.
> 
> Fixes: 2d81a24a74e5 ("driver: reset: th1520-aon: add driver for
> poweroff/reboot via AON FW")
> Signed-off-by: Han Gao <gaohan@iscas.ac.cn>
> ---
>  drivers/power/reset/th1520-aon-reboot.c | 49
> +++++++++++++++++++++++----------
>  1 file changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/power/reset/th1520-aon-reboot.c
> b/drivers/power/reset/th1520-aon-reboot.c
> index
> ec249667a0ffd74829357d57b9e63c3524ea3759..ace70491834dc9a63ed543427e4
> 0e4cbc0a9388f 100644
> --- a/drivers/power/reset/th1520-aon-reboot.c
> +++ b/drivers/power/reset/th1520-aon-reboot.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/auxiliary_bus.h>
> +#include <linux/delay.h>
>  #include <linux/firmware/thead/thead,th1520-aon.h>
>  #include <linux/module.h>
>  #include <linux/notifier.h>
> @@ -14,37 +15,46 @@
>  #include <linux/slab.h>
>  
>  #define TH1520_AON_REBOOT_PRIORITY 200
> +/* Allow an accepted asynchronous command to take effect before
> fallback. */
> +#define TH1520_AON_REBOOT_TIMEOUT_MS 1000
>  
>  struct th1520_aon_msg_empty_body {
>  	struct th1520_aon_rpc_msg_hdr hdr;
>  	u16 reserved[12];
>  } __packed __aligned(1);
>  
> -static int th1520_aon_pwroff_handler(struct sys_off_data *data)
> +static int th1520_aon_reboot_request(struct sys_off_data *data, u8
> func)
>  {
>  	struct th1520_aon_chan *aon_chan = data->cb_data;
>  	struct th1520_aon_msg_empty_body msg = {};
> +	int ret;
>  
>  	msg.hdr.svc = TH1520_AON_RPC_SVC_WDG;
> -	msg.hdr.func = TH1520_AON_WDG_FUNC_POWER_OFF;
> +	msg.hdr.func = func;
>  	msg.hdr.size = TH1520_AON_RPC_MSG_NUM;
>  
> -	th1520_aon_call_rpc(aon_chan, &msg);
> +	ret = th1520_aon_call_rpc(aon_chan, &msg);
> +	if (ret)
> +		dev_err(data->dev, "AON WDG command %u failed:
> %d\n", func, ret);
> +	else
> +		msleep(TH1520_AON_REBOOT_TIMEOUT_MS);
>  
>  	return NOTIFY_DONE;
>  }
>  
> -static int th1520_aon_restart_handler(struct sys_off_data *data)
> +static int th1520_aon_pwroff_handler(struct sys_off_data *data)
>  {
> -	struct th1520_aon_chan *aon_chan = data->cb_data;
> -	struct th1520_aon_msg_empty_body msg = {};
> -
> -	msg.hdr.svc = TH1520_AON_RPC_SVC_WDG;
> -	msg.hdr.func = TH1520_AON_WDG_FUNC_RESTART;
> -	msg.hdr.size = TH1520_AON_RPC_MSG_NUM;
> +	return th1520_aon_reboot_request(data,
> TH1520_AON_WDG_FUNC_POWER_OFF);
> +}
>  
> -	th1520_aon_call_rpc(aon_chan, &msg);
> +static int th1520_aon_restart_handler(struct sys_off_data *data)
> +{
> +	return th1520_aon_reboot_request(data,
> TH1520_AON_WDG_FUNC_RESTART);
> +}
>  
> +static int th1520_aon_pwroff_failed(struct sys_off_data *data)
> +{
> +	dev_err(data->dev, "AON did not power off the system\n");
>  	return NOTIFY_DONE;
>  }
>  
> @@ -54,8 +64,11 @@ static int th1520_aon_reboot_probe(struct
> auxiliary_device *adev,
>  	struct device *dev = &adev->dev;
>  	int ret;
>  
> -	/* Expect struct th1520_aon_chan to be passed via
> platform_data */
> -	ret = devm_register_sys_off_handler(dev,
> SYS_OFF_MODE_POWER_OFF,
> +	/*
> +	 * RPC takes a mutex and waits for mailbox interrupts. Send
> after device
> +	 * shutdown, but before syscore shutdown and the final
> atomic callbacks.
> +	 */
> +	ret = devm_register_sys_off_handler(dev,
> SYS_OFF_MODE_POWER_OFF_PREPARE,

Interestingly I tried to find usage of this key in the kernel, and
found that the macsmc-reboot driver has a "enter_atomic" operation.

I don't know whether it's a overdesign.

The imx_rproc driver even creates its own PREPARE handler that "setup
mailbox to non-blocking mode".

Maybe it's better to replicate the behavior of macsmc / imx_rproc in
the AON driver?

>  					   
> TH1520_AON_REBOOT_PRIORITY,
>  					   
> th1520_aon_pwroff_handler,
>  					    adev-
> >dev.platform_data);
> @@ -65,7 +78,7 @@ static int th1520_aon_reboot_probe(struct
> auxiliary_device *adev,
>  		return ret;
>  	}
>  
> -	ret = devm_register_sys_off_handler(dev,
> SYS_OFF_MODE_RESTART,
> +	ret = devm_register_sys_off_handler(dev,
> SYS_OFF_MODE_RESTART_PREPARE,
>  					   
> TH1520_AON_REBOOT_PRIORITY,
>  					   
> th1520_aon_restart_handler,
>  					    adev-
> >dev.platform_data);
> @@ -75,7 +88,13 @@ static int th1520_aon_reboot_probe(struct
> auxiliary_device *adev,
>  		return ret;
>  	}
>  
> -	return 0;
> +	/*
> +	 * Advertise power-off capability to kernel_can_power_off().
> This final
> +	 * callback must not send RPCs; let other providers try if
> AON failed.
> +	 */
> +	return devm_register_sys_off_handler(dev,
> SYS_OFF_MODE_POWER_OFF,
> +					    
> TH1520_AON_REBOOT_PRIORITY,
> +					    
> th1520_aon_pwroff_failed, dev_get_platdata(dev));

I don't know whether such a handler is necessary when the real power
operation is done in PREPARE handler. The odroid-go-ultra-poweroff
driver, which also (ab?)uses PREPARE handler for powering off, doesn't
register a non-PREPARE handler.

Thanks,
Icenowy

>  }
>  
>  static const struct auxiliary_device_id th1520_aon_reboot_id_table[]
> = {
> 
> ---
> base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
> change-id: 20260916-fix-xuantieaon-1ea016a22526
> 
> Best regards,

Re: [PATCH] power: reset: th1520-aon: send RPCs from sleepable handlers

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 6327 bytes --]

Hi,

On Wed, Sep 16, 2026 at 03:45:52PM +0800, Icenowy Zheng wrote:
> 在 2026-09-16三的 14:55 +0800，Han Gao写道：
> > The AON RPC takes a mutex and waits for mailbox interrupts. The final
> > sys-off handlers run on atomic notifier chains, where this can sleep
> > with interrupts disabled.
> > 
> > Send the requests from the preparation handlers before syscore
> > shutdown.
> > Retain a final power-off handler to advertise capability and report
> > failure without issuing another RPC. Let other providers handle
> > fallback.
> > 
> > Report RPC errors and allow one second after a successful return for
> > the
> > asynchronous operation to take effect. This is a software fallback
> > timeout, not a hardware timing requirement.
> > 
> > Fixes: 2d81a24a74e5 ("driver: reset: th1520-aon: add driver for
> > poweroff/reboot via AON FW")
> > Signed-off-by: Han Gao <gaohan@iscas.ac.cn>
> > ---
> >  drivers/power/reset/th1520-aon-reboot.c | 49
> > +++++++++++++++++++++++----------
> >  1 file changed, 34 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/power/reset/th1520-aon-reboot.c
> > b/drivers/power/reset/th1520-aon-reboot.c
> > index
> > ec249667a0ffd74829357d57b9e63c3524ea3759..ace70491834dc9a63ed543427e4
> > 0e4cbc0a9388f 100644
> > --- a/drivers/power/reset/th1520-aon-reboot.c
> > +++ b/drivers/power/reset/th1520-aon-reboot.c
> > @@ -6,6 +6,7 @@
> >   */
> >  
> >  #include <linux/auxiliary_bus.h>
> > +#include <linux/delay.h>
> >  #include <linux/firmware/thead/thead,th1520-aon.h>
> >  #include <linux/module.h>
> >  #include <linux/notifier.h>
> > @@ -14,37 +15,46 @@
> >  #include <linux/slab.h>
> >  
> >  #define TH1520_AON_REBOOT_PRIORITY 200
> > +/* Allow an accepted asynchronous command to take effect before
> > fallback. */
> > +#define TH1520_AON_REBOOT_TIMEOUT_MS 1000
> >  
> >  struct th1520_aon_msg_empty_body {
> >  	struct th1520_aon_rpc_msg_hdr hdr;
> >  	u16 reserved[12];
> >  } __packed __aligned(1);
> >  
> > -static int th1520_aon_pwroff_handler(struct sys_off_data *data)
> > +static int th1520_aon_reboot_request(struct sys_off_data *data, u8
> > func)
> >  {
> >  	struct th1520_aon_chan *aon_chan = data->cb_data;
> >  	struct th1520_aon_msg_empty_body msg = {};
> > +	int ret;
> >  
> >  	msg.hdr.svc = TH1520_AON_RPC_SVC_WDG;
> > -	msg.hdr.func = TH1520_AON_WDG_FUNC_POWER_OFF;
> > +	msg.hdr.func = func;
> >  	msg.hdr.size = TH1520_AON_RPC_MSG_NUM;
> >  
> > -	th1520_aon_call_rpc(aon_chan, &msg);
> > +	ret = th1520_aon_call_rpc(aon_chan, &msg);
> > +	if (ret)
> > +		dev_err(data->dev, "AON WDG command %u failed:
> > %d\n", func, ret);
> > +	else
> > +		msleep(TH1520_AON_REBOOT_TIMEOUT_MS);
> >  
> >  	return NOTIFY_DONE;
> >  }
> >  
> > -static int th1520_aon_restart_handler(struct sys_off_data *data)
> > +static int th1520_aon_pwroff_handler(struct sys_off_data *data)
> >  {
> > -	struct th1520_aon_chan *aon_chan = data->cb_data;
> > -	struct th1520_aon_msg_empty_body msg = {};
> > -
> > -	msg.hdr.svc = TH1520_AON_RPC_SVC_WDG;
> > -	msg.hdr.func = TH1520_AON_WDG_FUNC_RESTART;
> > -	msg.hdr.size = TH1520_AON_RPC_MSG_NUM;
> > +	return th1520_aon_reboot_request(data,
> > TH1520_AON_WDG_FUNC_POWER_OFF);
> > +}
> >  
> > -	th1520_aon_call_rpc(aon_chan, &msg);
> > +static int th1520_aon_restart_handler(struct sys_off_data *data)
> > +{
> > +	return th1520_aon_reboot_request(data,
> > TH1520_AON_WDG_FUNC_RESTART);
> > +}
> >  
> > +static int th1520_aon_pwroff_failed(struct sys_off_data *data)
> > +{
> > +	dev_err(data->dev, "AON did not power off the system\n");
> >  	return NOTIFY_DONE;
> >  }
> >  
> > @@ -54,8 +64,11 @@ static int th1520_aon_reboot_probe(struct
> > auxiliary_device *adev,
> >  	struct device *dev = &adev->dev;
> >  	int ret;
> >  
> > -	/* Expect struct th1520_aon_chan to be passed via
> > platform_data */
> > -	ret = devm_register_sys_off_handler(dev,
> > SYS_OFF_MODE_POWER_OFF,
> > +	/*
> > +	 * RPC takes a mutex and waits for mailbox interrupts. Send
> > after device
> > +	 * shutdown, but before syscore shutdown and the final
> > atomic callbacks.
> > +	 */
> > +	ret = devm_register_sys_off_handler(dev,
> > SYS_OFF_MODE_POWER_OFF_PREPARE,
> 
> Interestingly I tried to find usage of this key in the kernel, and
> found that the macsmc-reboot driver has a "enter_atomic" operation.
> 
> I don't know whether it's a overdesign.
> 
> The imx_rproc driver even creates its own PREPARE handler that "setup
> mailbox to non-blocking mode".
> 
> Maybe it's better to replicate the behavior of macsmc / imx_rproc in
> the AON driver?
> 
> >  					   
> > TH1520_AON_REBOOT_PRIORITY,
> >  					   
> > th1520_aon_pwroff_handler,
> >  					    adev-
> > >dev.platform_data);
> > @@ -65,7 +78,7 @@ static int th1520_aon_reboot_probe(struct
> > auxiliary_device *adev,
> >  		return ret;
> >  	}
> >  
> > -	ret = devm_register_sys_off_handler(dev,
> > SYS_OFF_MODE_RESTART,
> > +	ret = devm_register_sys_off_handler(dev,
> > SYS_OFF_MODE_RESTART_PREPARE,
> >  					   
> > TH1520_AON_REBOOT_PRIORITY,
> >  					   
> > th1520_aon_restart_handler,
> >  					    adev-
> > >dev.platform_data);
> > @@ -75,7 +88,13 @@ static int th1520_aon_reboot_probe(struct
> > auxiliary_device *adev,
> >  		return ret;
> >  	}
> >  
> > -	return 0;
> > +	/*
> > +	 * Advertise power-off capability to kernel_can_power_off().
> > This final
> > +	 * callback must not send RPCs; let other providers try if
> > AON failed.
> > +	 */
> > +	return devm_register_sys_off_handler(dev,
> > SYS_OFF_MODE_POWER_OFF,
> > +					    
> > TH1520_AON_REBOOT_PRIORITY,
> > +					    
> > th1520_aon_pwroff_failed, dev_get_platdata(dev));
> 
> I don't know whether such a handler is necessary when the real power
> operation is done in PREPARE handler. The odroid-go-ultra-poweroff
> driver, which also (ab?)uses PREPARE handler for powering off, doesn't
> register a non-PREPARE handler.

The real operation shouldn't be done in the PREPARE handler as that
results in the machine loosing power before it went through the
whole shutdown/reboot process.

Greetings,

-- Sebastian

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