Re: [PATCH v2 3/6] soc/tegra: pmc: Specify restart mode

[Benjamin Bara <bbara93@gmail.com>]

Hi Dmitry,

thanks for your feedback!

On Tue, 15 Aug 2023 at 00:38, Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:
> On 8/9/23 22:24, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > The current restart handler registration does not specify whether the
> > restart is a cold or a warm one. Now, as do_kernel_restart() knows about
> > the type, the priorization is implicitly done (cold restarts are
> > executed first) and the reboot_mode kernel parameter (which is currently
> > mostly ignored) can be respected.
> >
> > Acked-by: Thierry Reding <treding@nvidia.com>
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > v2:
> > - improve commit message
> > ---
> >  drivers/soc/tegra/pmc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> > index 162f52456f65..4f42febb9b0f 100644
> > --- a/drivers/soc/tegra/pmc.c
> > +++ b/drivers/soc/tegra/pmc.c
> > @@ -2962,7 +2962,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
> >       }
> > 
> >       err = devm_register_sys_off_handler(&pdev->dev,
> > -                                         SYS_OFF_MODE_RESTART,
> > +                                         SYS_OFF_MODE_RESTART_WARM,
> >                                           SYS_OFF_PRIO_LOW,
> >                                           tegra_pmc_restart_handler, NULL);
> >       if (err) {
> >
>
> You have tegra-pmc restart handler that uses low priority. And then
> you're adding cold/warm handlers to tps65219 and pca9450 drivers with a
> default priorities. 

Exactly, but I guess it makes sense to also use the handler with default
priority for the pmc reboot. The reason I kept it low prio was because
there is a comment that PMC should be last resort, but the reason
applies to any other soft restart handler too. I will adapt in the next
version.

> Hence this cold/warm separation of handlers doesn't do any practical
> difference in yours case because tegra-pmc will never be used as it
> did before your changes?

The change is e.g. relevant for platforms without PMIC-based soft reset,
e.g. when the tps6586x is in use. AFAIK, there is no possibility to
actually do a soft reboot, as the hard reboot will always be executed
first. This also happens if I set the kernel parameter "reboot_mode"
(also available via SysFS) to "soft" and a soft restart handler is
registered.

> Previously you wanted to make tps6586x driver to skip the warm reboot,
> but you're not touching tps6586x in this patchset.

True, there might also be other affected patches which are currently not
in linux-next yet. Will adapt the tps6586x too and depend on the whole
series in the next version.

> There is no real problem that is solved by these patches?

I think another problem is if the user sets the "reboot_mode" to "cold",
but there is no cold handler registered (as it was the case for me with
the pca9450), a warning should indicate that. AFAIK, there is no
possibility in user-space to find out if a cold restart handler is
registered, there is just this knob which can be switched to "cold". I
can also add a SysFS entry with "supported_modes" and check if the new
"mode" is supported on a store.

My other idea was to add a flags field to the notifier_block which
indicates (in case of a reboot notifier) the supported reboot_modes of
the registered handler, but I guess other notifier_block users won't
really benefit from an additional field, therefore I decided to add a
second list instead.

Best regards
Benjamin