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@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.
> > > > >
> > > > > 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;
> > > >
> > > > 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@xxxxxxx>
> 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
> > > > >
> > > > >
