Message-ID: <3e130395-8352-4de6-a82e-52594ce1d11f@gmail.com>
Date: Tue, 18 Mar 2025 15:37:09 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Sebastian Reichel <sre@...nel.org>,
 Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
 Benson Leung <bleung@...omium.org>, Tzung-Bi Shih <tzungbi@...nel.org>,
 Daniel Lezcano <daniel.lezcano@...aro.org>, chrome-platform@...ts.linux.dev,
 Ahmad Fatoum <a.fatoum@...gutronix.de>, "Rafael J. Wysocki"
 <rafael@...nel.org>, linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 kernel@...gutronix.de, Søren Andersen <san@...v.dk>,
 Guenter Roeck <groeck@...omium.org>, Zhang Rui <rui.zhang@...el.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Lukasz Luba <lukasz.luba@....com>
Subject: Re: [PATCH v7 3/7] power: reset: Introduce PSCR Recording Framework
 for Non-Volatile Storage

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@pengutronix.de
> https://lore.kernel.org/all/20250310102229.381887-1-o.rempel@pengutronix.de
> 

Powered by blists - more mailing lists