From: Peng Fan <peng.fan@nxp.com>
The i.MX7ULP Cortex-A7 is under control of Cortex-M4. The i.MX7ULP Linux
poweroff and restart rely on rpmsg driver to send a message to Cortex-M4
firmware. Then Cortex-A7 could poweroff or restart by Cortex-M4 to
configure the i.MX7ULP power controller properly.
However the reboot and restart kernel common code use atomic notifier,
so with blocking tx mailbox will trigger kernel dump, because of
blocking mailbox will use wait_for_completion_timeout. In such case,
linux no need to wait for completion.
Current patch is to use non-blocking tx mailbox channel when system
is going to poweroff or restart.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
drivers/remoteproc/imx_rproc.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 01cf1dfb2e87..e1abf110abc9 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -18,6 +18,7 @@
#include <linux/of_reserved_mem.h>
#include <linux/platform_device.h>
#include <linux/pm_domain.h>
+#include <linux/reboot.h>
#include <linux/regmap.h>
#include <linux/remoteproc.h>
#include <linux/workqueue.h>
@@ -114,6 +115,7 @@ struct imx_rproc {
u32 entry; /* cpu start address */
u32 core_index;
struct dev_pm_domain_list *pd_list;
+ struct sys_off_data data;
};
static const struct imx_rproc_att imx_rproc_att_imx93[] = {
@@ -1050,6 +1052,22 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
return 0;
}
+static int imx_rproc_sys_off_handler(struct sys_off_data *data)
+{
+ struct rproc *rproc = data->cb_data;
+ int ret;
+
+ imx_rproc_free_mbox(rproc);
+
+ ret = imx_rproc_xtr_mbox_init(rproc, false);
+ if (ret) {
+ dev_err(&rproc->dev, "Failed to request non-blocking mbox\n");
+ return NOTIFY_BAD;
+ }
+
+ return NOTIFY_DONE;
+}
+
static int imx_rproc_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1104,6 +1122,24 @@ static int imx_rproc_probe(struct platform_device *pdev)
if (rproc->state != RPROC_DETACHED)
rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot");
+ if (of_device_is_compatible(dev->of_node, "fsl,imx7ulp-cm4")) {
+ ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE,
+ SYS_OFF_PRIO_DEFAULT,
+ imx_rproc_sys_off_handler, rproc);
+ if (ret) {
+ dev_err(dev, "register power off handler failure\n");
+ goto err_put_clk;
+ }
+
+ ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART_PREPARE,
+ SYS_OFF_PRIO_DEFAULT,
+ imx_rproc_sys_off_handler, rproc);
+ if (ret) {
+ dev_err(dev, "register restart handler failure\n");
+ goto err_put_clk;
+ }
+ }
+
ret = rproc_add(rproc);
if (ret) {
dev_err(dev, "rproc_add failed\n");
--
2.37.1On Fri, Jul 19, 2024 at 04:49:04PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> The i.MX7ULP Cortex-A7 is under control of Cortex-M4. The i.MX7ULP Linux
> poweroff and restart rely on rpmsg driver to send a message to Cortex-M4
> firmware. Then Cortex-A7 could poweroff or restart by Cortex-M4 to
> configure the i.MX7ULP power controller properly.
>
> However the reboot and restart kernel common code use atomic notifier,
> so with blocking tx mailbox will trigger kernel dump, because of
> blocking mailbox will use wait_for_completion_timeout. In such case,
> linux no need to wait for completion.
>
> Current patch is to use non-blocking tx mailbox channel when system
> is going to poweroff or restart.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/remoteproc/imx_rproc.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 01cf1dfb2e87..e1abf110abc9 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -18,6 +18,7 @@
> #include <linux/of_reserved_mem.h>
> #include <linux/platform_device.h>
> #include <linux/pm_domain.h>
> +#include <linux/reboot.h>
> #include <linux/regmap.h>
> #include <linux/remoteproc.h>
> #include <linux/workqueue.h>
> @@ -114,6 +115,7 @@ struct imx_rproc {
> u32 entry; /* cpu start address */
> u32 core_index;
> struct dev_pm_domain_list *pd_list;
> + struct sys_off_data data;
What is this for? I don't see it used in this patch.
> };
>
> static const struct imx_rproc_att imx_rproc_att_imx93[] = {
> @@ -1050,6 +1052,22 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
> return 0;
> }
>
> +static int imx_rproc_sys_off_handler(struct sys_off_data *data)
> +{
> + struct rproc *rproc = data->cb_data;
> + int ret;
> +
> + imx_rproc_free_mbox(rproc);
> +
> + ret = imx_rproc_xtr_mbox_init(rproc, false);
> + if (ret) {
> + dev_err(&rproc->dev, "Failed to request non-blocking mbox\n");
> + return NOTIFY_BAD;
> + }
> +
> + return NOTIFY_DONE;
> +}
> +
> static int imx_rproc_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -1104,6 +1122,24 @@ static int imx_rproc_probe(struct platform_device *pdev)
> if (rproc->state != RPROC_DETACHED)
> rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot");
>
> + if (of_device_is_compatible(dev->of_node, "fsl,imx7ulp-cm4")) {
> + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE,
> + SYS_OFF_PRIO_DEFAULT,
> + imx_rproc_sys_off_handler, rproc);
Why does the mailbox needs to be set up again when the system is going down...
> + if (ret) {
> + dev_err(dev, "register power off handler failure\n");
> + goto err_put_clk;
> + }
> +
> + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART_PREPARE,
> + SYS_OFF_PRIO_DEFAULT,
> + imx_rproc_sys_off_handler, rproc);
... and why does it need to be free'd when the system is going up?
> + if (ret) {
> + dev_err(dev, "register restart handler failure\n");
> + goto err_put_clk;
> + }
> + }
> +
> ret = rproc_add(rproc);
> if (ret) {
> dev_err(dev, "rproc_add failed\n");
>
> --
> 2.37.1
>
>
Hello Mathieu,
I've talked to Peng and if my understanding is correct I think the patch is OK.
Maybe we can split the patch in two:
* first, adding the power off callback with explanations.
* second, adding the restart callback with explanations.
And also add a more detailed explanation.
Power off and restart are totally different operations and are not complementary
as I thought in the beginning. There are not like suspend/resume for example.
> > static int imx_rproc_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > @@ -1104,6 +1122,24 @@ static int imx_rproc_probe(struct platform_device *pdev)
> > if (rproc->state != RPROC_DETACHED)
> > rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot");
> >
> > + if (of_device_is_compatible(dev->of_node, "fsl,imx7ulp-cm4")) {
> > + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE,
> > + SYS_OFF_PRIO_DEFAULT,
> > + imx_rproc_sys_off_handler, rproc);
>
> Why does the mailbox needs to be set up again when the system is going down...
Scenario: We call Linux *shutdown -P * command to power off the machine.
At this point mailbox TX operation is configured as *blocking*. Power
off is done via
an atomic notifier call which doesn't allow blocking. If we do so we
will endup in a kernel crash.
So, at this moment we setup again the mailboxes configuring them with
*non-blocking* option.
>
> > + if (ret) {
> > + dev_err(dev, "register power off handler failure\n");
> > + goto err_put_clk;
> > + }
> > +
> > + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART_PREPARE,
> > + SYS_OFF_PRIO_DEFAULT,
> > + imx_rproc_sys_off_handler, rproc);
>
> ... and why does it need to be free'd when the system is going up?
System is not going up here. System is running and we do a reboot.
Scenario: We call Linux *shutdown -r* command to reboot the machine.
Similarly, mailboxes are already set and configured as *blocking*. We
cannot use the mailboxes
as they are because reboot is done via an atomic notifier which if we
call a blocking function it will endup in crash.
So, we need to free the existing mailbox and create new ones with the
*non-blocking* options.
I think this is really fair to me. The one thing, I admit we must work
on, create a better commit message.
What do you say? Does this work for you?
Thanks a lot for your help!
On Wed, 21 Aug 2024 at 02:32, Daniel Baluta <daniel.baluta@gmail.com> wrote:
>
> Hello Mathieu,
>
> I've talked to Peng and if my understanding is correct I think the patch is OK.
> Maybe we can split the patch in two:
> * first, adding the power off callback with explanations.
> * second, adding the restart callback with explanations.
>
> And also add a more detailed explanation.
>
> Power off and restart are totally different operations and are not complementary
> as I thought in the beginning. There are not like suspend/resume for example.
>
> > > static int imx_rproc_probe(struct platform_device *pdev)
> > > {
> > > struct device *dev = &pdev->dev;
> > > @@ -1104,6 +1122,24 @@ static int imx_rproc_probe(struct platform_device *pdev)
> > > if (rproc->state != RPROC_DETACHED)
> > > rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot");
> > >
> > > + if (of_device_is_compatible(dev->of_node, "fsl,imx7ulp-cm4")) {
> > > + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE,
> > > + SYS_OFF_PRIO_DEFAULT,
> > > + imx_rproc_sys_off_handler, rproc);
> >
> > Why does the mailbox needs to be set up again when the system is going down...
>
> Scenario: We call Linux *shutdown -P * command to power off the machine.
>
> At this point mailbox TX operation is configured as *blocking*. Power
> off is done via
> an atomic notifier call which doesn't allow blocking. If we do so we
> will endup in a kernel crash.
>
> So, at this moment we setup again the mailboxes configuring them with
> *non-blocking* option.
>
> >
> > > + if (ret) {
> > > + dev_err(dev, "register power off handler failure\n");
> > > + goto err_put_clk;
> > > + }
> > > +
> > > + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART_PREPARE,
> > > + SYS_OFF_PRIO_DEFAULT,
> > > + imx_rproc_sys_off_handler, rproc);
> >
> > ... and why does it need to be free'd when the system is going up?
>
> System is not going up here. System is running and we do a reboot.
>
Ah! This is still on the downward path - I thought
"SYS_OFF_MODE_RESTART_PREPARE" was associated with the upward path,
when the system is restarted after a shutdown or a reboot. That is
where the confusion came from.
> Scenario: We call Linux *shutdown -r* command to reboot the machine.
>
> Similarly, mailboxes are already set and configured as *blocking*. We
> cannot use the mailboxes
> as they are because reboot is done via an atomic notifier which if we
> call a blocking function it will endup in crash.
>
> So, we need to free the existing mailbox and create new ones with the
> *non-blocking* options.
>
> I think this is really fair to me. The one thing, I admit we must work
> on, create a better commit message.
>
> What do you say? Does this work for you?
>
Things are clear now and I agree with the implementation. No need for
two separate patches, just a re-worked changelog.
Thanks,
Mathieu
> Thanks a lot for your help!
© 2016 - 2025 Red Hat, Inc.