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

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(-)

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

Previously you wanted to make tps6586x driver to skip the warm reboot,
but you're not touching tps6586x in this patchset. There is no real
problem that is solved by these patches?

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