mbox series

[RFC,v1,0/7] Introduction of PSCR Framework and Related Components

Message ID 20240119132521.3609945-1-o.rempel@pengutronix.de
Headers show
Series Introduction of PSCR Framework and Related Components | expand

Message

Oleksij Rempel Jan. 19, 2024, 1:25 p.m. UTC
Hello all,

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.

Oleksij Rempel (7):
  dt-bindings: power: reset: add generic PSCR binding trackers
  power: reset: Introduce PSCR Tracking Framework for Non-Volatile
    Storage
  dt-bindings: power: reset: add bindings for NVMEM hardware storing
    PSCR Data
  nvmem: provide consumer access to cell size metrics
  power: reset: add PSCR NVMEM Driver for Storing Power State Change
    Reasons
  regulator: set Power State Change Reason before
    hw_protection_shutdown()
  thermal: core: set Power State Change Reason before
    hw_protection_shutdown()

 .../bindings/power/reset/pscr-nvmem.yaml      |  54 ++++
 .../devicetree/bindings/power/reset/pscr.yaml |  51 ++++
 drivers/nvmem/core.c                          |  25 ++
 drivers/power/reset/Kconfig                   |  30 ++
 drivers/power/reset/Makefile                  |   2 +
 drivers/power/reset/pscr-nvmem.c              | 100 +++++++
 drivers/power/reset/pscr.c                    | 259 ++++++++++++++++++
 drivers/regulator/core.c                      |   6 +
 drivers/thermal/thermal_core.c                |   2 +
 include/linux/nvmem-consumer.h                |   7 +
 include/linux/pscr.h                          |  40 +++
 11 files changed, 576 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/pscr-nvmem.yaml
 create mode 100644 Documentation/devicetree/bindings/power/reset/pscr.yaml
 create mode 100644 drivers/power/reset/pscr-nvmem.c
 create mode 100644 drivers/power/reset/pscr.c
 create mode 100644 include/linux/pscr.h

Comments

Rob Herring (Arm) Jan. 19, 2024, 2:50 p.m. UTC | #1
On Fri, 19 Jan 2024 14:25:15 +0100, Oleksij Rempel wrote:
> Add binding for Power State Change Reason (PSCR) subsystem
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/power/reset/pscr.yaml | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/pscr.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/reset/pscr.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
 	 $id: http://devicetree.org/schemas/power/state-change/pscr.yaml
 	file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/reset/pscr.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240119132521.3609945-2-o.rempel@pengutronix.de

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Oleksij Rempel Jan. 19, 2024, 5:54 p.m. UTC | #2
On Fri, Jan 19, 2024 at 11:28:38AM -0600, Rob Herring wrote:
> On Fri, Jan 19, 2024 at 02:25:15PM +0100, Oleksij Rempel wrote:
> > Add binding for Power State Change Reason (PSCR) subsystem
> 
> Why? 
> 
> How is this different from the reboot reason binding?

I was not able to find "reboot reason", you probably refer to "reboot
mode".

Reboot Mode:
 Purpose: Dictates how the system should reboot (e.g., normal, recovery,
   bootloader).
 Usage: Provides instructions for the next boot mode.
 Scenario: Utilized in planned reboots or software-triggered reset scenarios.

PSCR (Power State Change Reasons):
 Purpose: Logs the reason behind a power state change (e.g., voltage drop,
   over-temperature).
 Usage: Used for rapid logging and post-event analysis, potentially informing
   automatic decision-making in subsequent boots.
 Scenario: Critical in abrupt power-down situations where immediate, detailed
   decision-making is not possible.

Regards,
Oleksij
Rafael J. Wysocki Jan. 19, 2024, 8:11 p.m. UTC | #3
On Fri, Jan 19, 2024 at 8:34 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> On Fri, Jan 19, 2024 at 07:34:26PM +0100, Rafael J. Wysocki wrote:
> > On Fri, Jan 19, 2024 at 2:25 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > >
> > > Store the state change reason to some black box for later investigation.
> >
> > Seriously?
> >
> > What black box, where, how this is useful and who is going to use it,
> > pretty please.
>
> The 'black box' refers to a non-volatile memory (NVMEM) cell used by the
> Power State Change Reasons (PSCR) framework. This cell stores reasons
> for sudden power state changes, like voltage drops or over-temperature
> events. This data is invaluable for post-mortem analysis to understand
> system failures or abrupt shutdowns. It's particularly useful for
> systems where PMICs or watchdogs cannot record such events. The data can
> inform recovery routines in the bootloader or early kernel stages during
> subsequent boots, enhancing system reliability and aiding in debugging
> and diagnostics.

OK, so please add all of the above to the patch changelog.
Sebastian Reichel Jan. 19, 2024, 11:19 p.m. UTC | #4
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.

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.

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

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

-- Sebastian
Oleksij Rempel Jan. 21, 2024, 6:56 a.m. UTC | #5
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