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!