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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> Subject: Re: [PATCH 6/6] remoteproc: imx_rproc: handle system off for
> i.MX7ULP
> 
> On Fri, Jul 12, 2024 at 04:34:59PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@xxxxxxx>
> >
> > 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.
> >
> > Reviewed-by: Jacky Bai <ping.bai@xxxxxxx>
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > ---
> >  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"))
> {
> 
> I don't suggest check compatible string. It'd better add a field  in
> imx_rproc_dcfg, such as need_sys_off
> 
> 	if (dcfg->need_sys_off) {
> 		...
> 	}
> 
> If there are new compatible string added, just need set need_sys_off to
> true in driver data.

Could we delay the change when there is really new chips need this?
The downstream commit time is " Date:   Tue Dec 6 17:10:14 2022",
In the past days, I not see other platforms require this.

Mathieu, which do you prefer? add need_sys_off or keep current
approach?

Thanks,
Peng.

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





[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Photo Sharing]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux