Re: [RFC PATCH v1 0/7] Introduction of PSCR Framework and Related Components

[Oleksij Rempel]

Hi,

On Sat, Jan 20, 2024 at 12:19:09AM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Jan 19, 2024 at 02:25:14PM +0100, Oleksij Rempel wrote:
> > This patch series introduces the Power State Change Reasons (PSCR)
> > tracking framework and its related components into the kernel. The PSCR
> > framework is designed for systems where traditional methods of storing
> > power state change reasons, like PMICs or watchdogs, are inadequate. It
> > provides a structured way to store reasons for system shutdowns and
> > reboots, such as under-voltage or software-triggered events, in
> > non-volatile hardware storage.
> > 
> > These changes are critical for systems requiring detailed postmortem
> > analysis and where immediate power-down scenarios limit traditional
> > storage options. The framework also assists bootloaders and early-stage
> > system components in making informed recovery decisions.
> 
> A couple of things come to my mind:
> 
> 1. Do we need the DT based reason-string-to-integer mapping? Can we
>    just use a fixed mapping instead? It simplifies the binding a
>    lot. With that the generic part could be dropped completely.

The project I'm working is using RTC for storage. The RTC device in
question provides 8 bits, 3 bits are assigned for PSCR.
Currently all reasons provided in this patch set would fit int to 3 bits,
but soon or later it may expand.

> 2. I would expect the infrastructure to read and clear the reason
>    during boot. If e.g. a watchdog triggers a reset you will otherwise
>    get an incorrect value.

Hm.. good point. I'll set a value on the boot that there is currently no
attempt to shutdown at all. PSCR works only for software assisted
shutdown/reboot. It should extend, not replace PMIC or watchdog detected
reasons.

> 3. The reason is only stored, but not used? We have a sysfs ABI to
>    expose the reboot reason to userspace since half a year ago, see
>    d40befed9a58 (power: reset: at91-reset: add sysfs interface to
>    the power on reason).

ACK. I'll add sysfs support.
For my use case, the user is the bootloader. The is one of reasons why
DT is used for mappings, this is the stable ABI between this systems.

> 4. Available options should be synced with the list in
>    include/linux/power/power_on_reason.h

ACK

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |