Re: [PATCH v2 0/2] remoteproc: imx_rproc: support non-blocking tx for i.MX7ULP

Re: [PATCH v2 0/2] remoteproc: imx_rproc: support non-blocking tx for i.MX7ULP

[Daniel Baluta]

On Fri, Jul 19, 2024 at 11:40 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> 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.
>
> patch 1 is to support non-blocking tx mailbox channel
> patch 2 is to switch to non-blocking tx for system poweroff or restart.
>
> Based on patchset:
> https://lore.kernel.org/all/20240719-imx_rproc-v2-0-10d0268c7eb1@nxp.com/
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

Series looks good to me.

Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>

[PATCH v2 0/2] remoteproc: imx_rproc: support non-blocking tx for i.MX7ULP

[Peng Fan (OSS)]

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.

patch 1 is to support non-blocking tx mailbox channel
patch 2 is to switch to non-blocking tx for system poweroff or restart.

Based on patchset:
https://lore.kernel.org/all/20240719-imx_rproc-v2-0-10d0268c7eb1@nxp.com/

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Changes in v2:
- Separated patch 5,6 from v1
- Update commit patch 1 with adding "No functional changes"
- Link to v1: https://lore.kernel.org/r/20240712-imx_rproc-v1-0-7bcf6732d328@nxp.com

---
Peng Fan (2):
      remoteproc: imx_rproc: allow tx_block to be set
      remoteproc: imx_rproc: handle system off for i.MX7ULP

 drivers/remoteproc/imx_rproc.c | 48 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)
---
base-commit: 0de2e1d9af457b2eb010636d59dfef569659783a
change-id: 20240712-imx_rproc-25f3ab753c58

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

[PATCH v2 1/2] remoteproc: imx_rproc: allow tx_block to be set

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Current tx_block is set to true, but there is case that no need to wait
response. Linux just needs to send data to remote processor, so let's
allow tx_block could be set to false. No functional changes.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 552fccebf7e2..01cf1dfb2e87 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -90,7 +90,7 @@ struct imx_rproc_mem {
 #define ATT_CORE_MASK   0xffff
 #define ATT_CORE(I)     BIT((I))
 
-static int imx_rproc_xtr_mbox_init(struct rproc *rproc);
+static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block);
 static void imx_rproc_free_mbox(struct rproc *rproc);
 
 struct imx_rproc {
@@ -369,7 +369,7 @@ static int imx_rproc_start(struct rproc *rproc)
 	struct arm_smccc_res res;
 	int ret;
 
-	ret = imx_rproc_xtr_mbox_init(rproc);
+	ret = imx_rproc_xtr_mbox_init(rproc, true);
 	if (ret)
 		return ret;
 
@@ -629,7 +629,7 @@ static void imx_rproc_kick(struct rproc *rproc, int vqid)
 
 static int imx_rproc_attach(struct rproc *rproc)
 {
-	return imx_rproc_xtr_mbox_init(rproc);
+	return imx_rproc_xtr_mbox_init(rproc, true);
 }
 
 static int imx_rproc_detach(struct rproc *rproc)
@@ -794,7 +794,7 @@ static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
 	queue_work(priv->workqueue, &priv->rproc_work);
 }
 
-static int imx_rproc_xtr_mbox_init(struct rproc *rproc)
+static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block)
 {
 	struct imx_rproc *priv = rproc->priv;
 	struct device *dev = priv->dev;
@@ -817,7 +817,7 @@ static int imx_rproc_xtr_mbox_init(struct rproc *rproc)
 
 	cl = &priv->cl;
 	cl->dev = dev;
-	cl->tx_block = true;
+	cl->tx_block = tx_block;
 	cl->tx_tout = 100;
 	cl->knows_txdone = false;
 	cl->rx_callback = imx_rproc_rx_callback;
@@ -1083,7 +1083,7 @@ static int imx_rproc_probe(struct platform_device *pdev)
 
 	INIT_WORK(&priv->rproc_work, imx_rproc_vq_work);
 
-	ret = imx_rproc_xtr_mbox_init(rproc);
+	ret = imx_rproc_xtr_mbox_init(rproc, true);
 	if (ret)
 		goto err_put_wkq;
 

-- 
2.37.1

[PATCH v2 2/2] remoteproc: imx_rproc: handle system off for i.MX7ULP

[Peng Fan (OSS)]

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.1

Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off for i.MX7ULP

[Mathieu Poirier]

On 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
> 
> 

RE: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off for i.MX7ULP

[Peng Fan]

> Subject: Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off
> for i.MX7ULP
> 
> On 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.

Oh, it was added when I was developing this feature, but in the end
this seems not needed.

> 
> >  };
> >
> >  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...

As wrote in commit message:
"i.MX7ULP Linux poweroff and restart rely on rpmsg driver to send a
message," so need to set up mailbox in non-blocking way to send
a message to M4 side.

> 
> > +		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?


Sorry, I not get your point. The free is in imx_rproc_sys_off_handler.
During system booting, the mailbox is not freed.

Thanks,
Peng.

> 
> > +		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
> >
> >

Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off for i.MX7ULP

[Mathieu Poirier]

On Tue, Jul 30, 2024 at 08:06:22AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off
> > for i.MX7ULP
> > 
> > On 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.
> 
> Oh, it was added when I was developing this feature, but in the end
> this seems not needed.
> 
> > 
> > >  };
> > >
> > >  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...
> 
> As wrote in commit message:
> "i.MX7ULP Linux poweroff and restart rely on rpmsg driver to send a
> message," so need to set up mailbox in non-blocking way to send
> a message to M4 side.
> 
> > 
> > > +		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?
> 
> 
> Sorry, I not get your point. The free is in imx_rproc_sys_off_handler.
> During system booting, the mailbox is not freed.

Why is the same operation done at both startup and shutdown - that is not clear.

I am currently away from the office, returning on August 12th.  As such I will
not be following up on this thread until then.

> 
> Thanks,
> Peng.
> 
> > 
> > > +		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
> > >
> > >

RE: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off for i.MX7ULP

[Peng Fan]

> Subject: Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off
> for i.MX7ULP
> 
> On Tue, Jul 30, 2024 at 08:06:22AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system
> off
> > > for i.MX7ULP
> > >
> > > On 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.
> >
> > Oh, it was added when I was developing this feature, but in the end
> > this seems not needed.
> >
> > >
> > > >  };
> > > >
> > > >  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...
> >
> > As wrote in commit message:
> > "i.MX7ULP Linux poweroff and restart rely on rpmsg driver to send a
> > message," so need to set up mailbox in non-blocking way to send a
> > message to M4 side.
> >
> > >
> > > > +		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?
> >
> >
> > Sorry, I not get your point. The free is in imx_rproc_sys_off_handler.
> > During system booting, the mailbox is not freed.
> 
> Why is the same operation done at both startup and shutdown - that is
> not clear.

The below commit shows request/free done in startup and shutdown.
Hope this explains what you ask.

commit 99b142cf7191b08adcd23f700ea0a3d7dffdd0c1
Author: Peng Fan <peng.fan@nxp.com>
Date:   Fri Oct 21 12:15:25 2022 +0800

    remoteproc: imx_rproc: Request mbox channel later
    
    It is possible that when remote processor crash, the communication
    channel will be broken with garbage value in mailbox, such as
    when Linux is issuing a message through mailbox, remote processor
    crashes, we need free & rebuild the mailbox channels to make sure
    no garbage value in mailbox channels.
    
    So move the request/free to start/stop for managing remote procesosr in
    Linux, move to attach/detach for remote processor is out of control of
    Linux.
    
    Previous, we just request mbox when attach for CM4 boot early before
    Linux, but if mbox defer probe, remoteproc core will do resource cleanup
    and corrupt resource table for later probe.
    
    So move request mbox ealier and still keep mbox request when attach
    for self recovery case, but keep a check when request/free mbox.

> 
> I am currently away from the office, returning on August 12th.  As such
> I will not be following up on this thread until then.

sure. Thanks for letting me know.

Thanks,
Peng.

> 
> >
> > Thanks,
> > Peng.
> >
> > >
> > > > +		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
> > > >
> > > >

Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off for i.MX7ULP

[Mathieu Poirier]

On Fri, Aug 02, 2024 at 04:59:45AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off
> > for i.MX7ULP
> > 
> > On Tue, Jul 30, 2024 at 08:06:22AM +0000, Peng Fan wrote:
> > > > Subject: Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system
> > off
> > > > for i.MX7ULP
> > > >
> > > > On 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.
> > >
> > > Oh, it was added when I was developing this feature, but in the end
> > > this seems not needed.
> > >
> > > >
> > > > >  };
> > > > >
> > > > >  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...
> > >
> > > As wrote in commit message:
> > > "i.MX7ULP Linux poweroff and restart rely on rpmsg driver to send a
> > > message," so need to set up mailbox in non-blocking way to send a
> > > message to M4 side.
> > >
> > > >
> > > > > +		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?
> > >
> > >
> > > Sorry, I not get your point. The free is in imx_rproc_sys_off_handler.
> > > During system booting, the mailbox is not freed.
> > 
> > Why is the same operation done at both startup and shutdown - that is
> > not clear.
> 
> The below commit shows request/free done in startup and shutdown.
> Hope this explains what you ask.

Unfortunately it doesn't.  I just spent another hour trying to understand why
the same operations are carried out for both shutdown and restart without
success.  I am out of time for this patch and have to move on to other patchset
waiting to be reviewed.  I suggest you ask Daniel to help clarify the changelog
and comments in the code and submit another revision.

Thanks,
Mathieu

> 
> commit 99b142cf7191b08adcd23f700ea0a3d7dffdd0c1
> Author: Peng Fan <peng.fan@nxp.com>
> Date:   Fri Oct 21 12:15:25 2022 +0800
> 
>     remoteproc: imx_rproc: Request mbox channel later
>     
>     It is possible that when remote processor crash, the communication
>     channel will be broken with garbage value in mailbox, such as
>     when Linux is issuing a message through mailbox, remote processor
>     crashes, we need free & rebuild the mailbox channels to make sure
>     no garbage value in mailbox channels.
>     
>     So move the request/free to start/stop for managing remote procesosr in
>     Linux, move to attach/detach for remote processor is out of control of
>     Linux.
>     
>     Previous, we just request mbox when attach for CM4 boot early before
>     Linux, but if mbox defer probe, remoteproc core will do resource cleanup
>     and corrupt resource table for later probe.
>     
>     So move request mbox ealier and still keep mbox request when attach
>     for self recovery case, but keep a check when request/free mbox.
> 
> > 
> > I am currently away from the office, returning on August 12th.  As such
> > I will not be following up on this thread until then.
> 
> sure. Thanks for letting me know.
> 
> Thanks,
> Peng.
> 
> > 
> > >
> > > Thanks,
> > > Peng.
> > >
> > > >
> > > > > +		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
> > > > >
> > > > >

RE: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off for i.MX7ULP

[Peng Fan]

> Subject: Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off
> for i.MX7ULP
> 
> On Fri, Aug 02, 2024 at 04:59:45AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system
> off
> > > for i.MX7ULP
> > >
> > > On Tue, Jul 30, 2024 at 08:06:22AM +0000, Peng Fan wrote:
> > > > > Subject: Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle
> system
> > > off
> > > > > for i.MX7ULP
> > > > >
> > > > > On 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.
> > > >
> > > > Oh, it was added when I was developing this feature, but in the
> > > > end this seems not needed.
> > > >
> > > > >
> > > > > >  };
> > > > > >
> > > > > >  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...
> > > >
> > > > As wrote in commit message:
> > > > "i.MX7ULP Linux poweroff and restart rely on rpmsg driver to
> send
> > > > a message," so need to set up mailbox in non-blocking way to
> send
> > > > a message to M4 side.
> > > >
> > > > >
> > > > > > +		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?
> > > >
> > > >
> > > > Sorry, I not get your point. The free is in
> imx_rproc_sys_off_handler.
> > > > During system booting, the mailbox is not freed.
> > >
> > > Why is the same operation done at both startup and shutdown -
> that
> > > is not clear.
> >
> > The below commit shows request/free done in startup and shutdown.
> > Hope this explains what you ask.
> 
> Unfortunately it doesn't.  I just spent another hour trying to
> understand why the same operations are carried out for both
> shutdown and restart without success.  

Thanks for your time.
During system shutdown or system restart, we have other
drivers that needs send rpmsg message to M7 core, but the
system shutdown or system restart notifier callback not allow
sleeping, so we need reset the mailbox as non-blocking.

I am out of time for this patch
> and have to move on to other patchset waiting to be reviewed.  I
> suggest you ask Daniel to help clarify the changelog and comments in
> the code and submit another revision.

ok, I will ask Daniel for help to improve.

Thanks,
Peng.

> 
> Thanks,
> Mathieu
> 
> >
> > commit 99b142cf7191b08adcd23f700ea0a3d7dffdd0c1
> > Author: Peng Fan <peng.fan@nxp.com>
> > Date:   Fri Oct 21 12:15:25 2022 +0800
> >
> >     remoteproc: imx_rproc: Request mbox channel later
> >
> >     It is possible that when remote processor crash, the
> communication
> >     channel will be broken with garbage value in mailbox, such as
> >     when Linux is issuing a message through mailbox, remote
> processor
> >     crashes, we need free & rebuild the mailbox channels to make sure
> >     no garbage value in mailbox channels.
> >
> >     So move the request/free to start/stop for managing remote
> procesosr in
> >     Linux, move to attach/detach for remote processor is out of control
> of
> >     Linux.
> >
> >     Previous, we just request mbox when attach for CM4 boot early
> before
> >     Linux, but if mbox defer probe, remoteproc core will do resource
> cleanup
> >     and corrupt resource table for later probe.
> >
> >     So move request mbox ealier and still keep mbox request when
> attach
> >     for self recovery case, but keep a check when request/free mbox.
> >
> > >
> > > I am currently away from the office, returning on August 12th.  As
> > > such I will not be following up on this thread until then.
> >
> > sure. Thanks for letting me know.
> >
> > Thanks,
> > Peng.
> >
> > >
> > > >
> > > > Thanks,
> > > > Peng.
> > > >
> > > > >
> > > > > > +		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
> > > > > >
> > > > > >

Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off for i.MX7ULP

[Daniel Baluta]

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!

Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off for i.MX7ULP

[Mathieu Poirier]

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!