Re: [PATCH v7 3/7] power: reset: Introduce PSCR Recording Framework for Non-Volatile Storage

[Matti Vaittinen]

On 18/03/2025 15:19, Oleksij Rempel wrote:
> On Tue, Mar 18, 2025 at 01:01:30PM +0200, Matti Vaittinen wrote:
> > On 18/03/2025 11:47, Oleksij Rempel wrote:
> > > This commit introduces the Power State Change Reasons Recording (PSCRR)
> > > framework into the kernel. The framework is vital for systems where
> > > PMICs or watchdogs cannot provide information on power state changes. It
> > > stores reasons for system shutdowns and reboots, like under-voltage or
> > > software-triggered events, in non-volatile hardware storage. This
> > > approach is essential for postmortem analysis in scenarios where
> > > traditional storage methods (block devices, RAM) are not feasible. The
> > > framework aids bootloaders and early-stage system components in recovery
> > > decision-making, although it does not cover resets caused by hardware
> > > issues like system freezes or watchdog timeouts.
> >
> > We might want to rephrase this if we envision that boot reason could be read
> > from PMICs (or other devices able to store the boot reason) using PSCRR
> > interface. (Because a few PMICs can store the boot reason even for the
> > hardware initiated shutdowns like Watchdog or voltage/current protection).
>
> ack.
>
> > > It is primarily intended for controlled power
> > > +	  state transitions.
> > > +
> > > +	  If unsure, say N.
> > > diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> > > index 10782d32e1da..dbd6ae6b26a4 100644
> > > --- a/drivers/power/reset/Makefile
> > > +++ b/drivers/power/reset/Makefile
> > > @@ -32,6 +32,7 @@ obj-$(CONFIG_POWER_RESET_KEYSTONE) += keystone-reset.o
> > >    obj-$(CONFIG_POWER_RESET_SYSCON) += syscon-reboot.o
> > >    obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
> > >    obj-$(CONFIG_POWER_RESET_RMOBILE) += rmobile-reset.o
> > > +obj-$(CONFIG_PSCRR) += pscrr.o
> > >    obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o
> > >    obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
> > >    obj-$(CONFIG_POWER_RESET_SC27XX) += sc27xx-poweroff.o
> > > diff --git a/drivers/power/reset/pscrr.c b/drivers/power/reset/pscrr.c
> > > new file mode 100644
> > > index 000000000000..466eca0e4f7f
> > > --- /dev/null
> > > +++ b/drivers/power/reset/pscrr.c
> > > @@ -0,0 +1,417 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * pscrr_core.c - Core Power State Change Reason Recording
> > > + *
> > > + * This framework provides a method for recording the cause of the last system
> > > + * reboot, particularly in scenarios where **hardware protection events** (e.g.,
> > > + * undervoltage, overcurrent, thermal shutdown) force an immediate reset.
> >
> > Is this contradicting the Kconfig / commit message?
>
> There is so many redundant text, i already lost track of it.  What do
> you mean?

Sorry, I should've been more specific :)

I was just thinking that it may be hard to understand what type of 
events the PSCRR is able to detect. The commit message (which I thought 
might be rephrased a bit) said:


The
>>> framework aids bootloaders and early-stage system components in 
recovery
>>> decision-making, although it does not cover resets caused by hardware
>>> issues like system freezes or watchdog timeouts.

(What I got stuck was the "does not cover resets caused by hardware 
issues" - but maybe it's just my limited reading abilities because the 
sentence seems to say also: "like system freezes or watchdog timeouts". 
So perhaps there is no contradiction here.)

Kconfig says:

Note
+	  that this framework does not track hardware-induced resets, such as
+	  system freezes, watchdog timeouts, or sudden power losses without
+	  controlled shutdown. It is primarily intended for controlled power
+	  state transitions.

Here we however have:

"particularly in scenarios where **hardware protection events** (e.g.,
undervoltage, overcurrent, thermal shutdown) force an immediate reset."

So, I was wondering if there is a way to clarify what type of hardware 
protection events are covered, and what aren't. I know this is not easy, 
especially if we allow reset reasons from PMICs to be included.

The comment was a 'nit' so if you see no good way to improve, then 
please keep it as it is.

> > > Unlike
> > > + * traditional logging mechanisms that rely on block storage (e.g., NAND, eMMC),
> > > + * PSCRR ensures shutdown reasons are preserved in a way that survives power
> > > + * loss for later analysis.
> >
> > Here, the 'level of power-loss' plays a role, right? I assume some level of
> > power must be retained for the 'storage' to stay alive.
>
> Yes, on the system I'm working on, there is designed capacity and
> voltage drop detector to do $THINGS before the system will go off. To
> get the full picture you may take a look to following patches:
>
> https://lore.kernel.org/all/20231026144824.4065145-1-o.rempel@xxxxxxxxxxxxxx
> https://lore.kernel.org/all/20250310102229.381887-1-o.rempel@xxxxxxxxxxxxxx