| Message ID | 20240124122204.730370-1-o.rempel@pengutronix.de |
|---|---|
| Headers | show |
| Series | Introduction of PSCR Framework and Related Components | expand |
On 24/01/2024 13:21, Oleksij Rempel wrote: > Add binding for Power State Change Reason Recording (PSCRR) subsystem > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > .../bindings/power/reset/pscrr.yaml | 44 +++++++++++++++++++ > 1 file changed, 44 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/reset/pscrr.yaml > > diff --git a/Documentation/devicetree/bindings/power/reset/pscrr.yaml b/Documentation/devicetree/bindings/power/reset/pscrr.yaml > new file mode 100644 > index 000000000000..c8738b4930fe > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/reset/pscrr.yaml > @@ -0,0 +1,44 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/power/reset/pscrr.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Power State Change Reason (PSCR) Missing final R? > + > +maintainers: > + - Oleksij Rempel <o.rempel@pengutronix.de> > + > +description: Binding for devices responsible to store reasons for power state Line break after description: > + changes such as reboot and power-off. Reasons like unknown, under voltage, > + and over temperature are captured for diagnostic or automatic recovery > + purposes. > + > +properties: > + pscr-under-voltage: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: | > + Value to indicate an under-voltage condition of a system critical > + regulator as the reason for the power state change. I don't understand how it is supposed to work... unless you wrote binding for drivers. For drivers it would make sense, but that's another problem: binding is not for drivers. You also did not present here DTS with any actual device doing this. You just added a driver... Best regards, Krzysztof
On 24/01/2024 13:22, Oleksij Rempel wrote: > Add device tree bindings that describe hardware implementations of > Non-Volatile Memory (NVMEM) used for storing Power State Change Reasons > (PSCR). A nit, subject: drop second/last, redundant "bindings for". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > .../bindings/power/reset/pscrr-nvmem.yaml | 53 +++++++++++++++++++ > 1 file changed, 53 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/reset/pscrr-nvmem.yaml > > diff --git a/Documentation/devicetree/bindings/power/reset/pscrr-nvmem.yaml b/Documentation/devicetree/bindings/power/reset/pscrr-nvmem.yaml > new file mode 100644 > index 000000000000..779920dea283 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/reset/pscrr-nvmem.yaml > @@ -0,0 +1,53 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/power/reset/pscrr-nvmem.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Generic NVMEM Power State Change Reason Recorder > + > +maintainers: > + - Oleksij Rempel <o.rempel@pengutronix.de> > + > +description: This binding describes the Non-Volatile Memory (NVMEM) hardware Same comment and also: describe the hardware, not the binding. s/This binding describes/something useful/ > + that stores Power State Change Reasons (PSCR). > + > +allOf: > + - $ref: pscrr.yaml# > + > +properties: > + compatible: > + const: pscrr-nvmem > + So that's a driver :/. Maybe Rob will like it, but it's a no from me. Please come up with something really suiting DEVICES, not DRIVERS. > + nvmem-cells: > + description: | Do not need '|' unless you need to preserve formatting. > + A phandle pointing to the nvmem-cells node where the power state change > + reasons are stored. > + maxItems: 1 > + Best regards, Krzysztof
On Thu, Jan 25, 2024 at 11:57:18AM +0100, Krzysztof Kozlowski wrote: > On 24/01/2024 13:22, Oleksij Rempel wrote: > > Add device tree bindings that describe hardware implementations of > > Non-Volatile Memory (NVMEM) used for storing Power State Change Reasons > > (PSCR). > > + that stores Power State Change Reasons (PSCR). > > + > > +allOf: > > + - $ref: pscrr.yaml# > > + > > +properties: > > + compatible: > > + const: pscrr-nvmem > > + > > So that's a driver :/. Maybe Rob will like it, but it's a no from me. > Please come up with something really suiting DEVICES, not DRIVERS. If I understand your distinction between 'DEVICES' and 'DRIVERS' correctly, 'DEVICES' in the device tree context are meant to represent physical hardware components, while 'DRIVERS' refer to software abstractions of these components. However, there are numerous device tree instances, like software-based implementations for SPI, I2C, or GPIO, which could also be interpreted as 'DRIVERS' in the context of your email. Similarly, the binding for PSCRR represents functionality not fully implemented in hardware but supported by the hardware component of NVMEM, akin to how ramoops or other functionalities are represented. If I'm misunderstanding your distinction between 'DEVICES' and 'DRIVERS', please clarify with an example of how a proper binding should be implemented for a case like this. Regards, Oleksij
On 25/01/2024 18:11, Oleksij Rempel wrote: > On Thu, Jan 25, 2024 at 11:57:18AM +0100, Krzysztof Kozlowski wrote: >> On 24/01/2024 13:22, Oleksij Rempel wrote: >>> Add device tree bindings that describe hardware implementations of >>> Non-Volatile Memory (NVMEM) used for storing Power State Change Reasons >>> (PSCR). >>> + that stores Power State Change Reasons (PSCR). >>> + >>> +allOf: >>> + - $ref: pscrr.yaml# >>> + >>> +properties: >>> + compatible: >>> + const: pscrr-nvmem >>> + >> >> So that's a driver :/. Maybe Rob will like it, but it's a no from me. >> Please come up with something really suiting DEVICES, not DRIVERS. > > If I understand your distinction between 'DEVICES' and 'DRIVERS' > correctly, 'DEVICES' in the device tree context are meant to represent > physical hardware components, while 'DRIVERS' refer to software Yes. > abstractions of these components. However, there are numerous device > tree instances, like software-based implementations for SPI, I2C, or > GPIO, which could also be interpreted as 'DRIVERS' in the context of True. Yet they are still for physical interfaces. There is no DTS having some virtual I2C for a board which does not have I2C. > your email. Similarly, the binding for PSCRR represents functionality not > fully implemented in hardware but supported by the hardware component of > NVMEM, akin to how ramoops or other functionalities are represented. You don't need a binding for your case. Instantiate it whatever you wish - modprobe for example - and configure through approved kernel interfaces - sysfs for example. Best regards, Krzysztof
On 26/01/2024 17:51, Oleksij Rempel wrote: > On Fri, Jan 26, 2024 at 10:03:51AM +0100, Krzysztof Kozlowski wrote: >> On 25/01/2024 18:11, Oleksij Rempel wrote: >>> On Thu, Jan 25, 2024 at 11:57:18AM +0100, Krzysztof Kozlowski wrote: >>>> On 24/01/2024 13:22, Oleksij Rempel wrote: >>>>> Add device tree bindings that describe hardware implementations of >>>>> Non-Volatile Memory (NVMEM) used for storing Power State Change Reasons >>>>> (PSCR). >>>>> + that stores Power State Change Reasons (PSCR). >>>>> + >>>>> +allOf: >>>>> + - $ref: pscrr.yaml# >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + const: pscrr-nvmem >>>>> + >>>> >>>> So that's a driver :/. Maybe Rob will like it, but it's a no from me. >>>> Please come up with something really suiting DEVICES, not DRIVERS. >>> >>> If I understand your distinction between 'DEVICES' and 'DRIVERS' >>> correctly, 'DEVICES' in the device tree context are meant to represent >>> physical hardware components, while 'DRIVERS' refer to software >> >> Yes. >> >>> abstractions of these components. However, there are numerous device >>> tree instances, like software-based implementations for SPI, I2C, or >>> GPIO, which could also be interpreted as 'DRIVERS' in the context of >> >> True. Yet they are still for physical interfaces. There is no DTS having >> some virtual I2C for a board which does not have I2C. >> >>> your email. Similarly, the binding for PSCRR represents functionality not >>> fully implemented in hardware but supported by the hardware component of >>> NVMEM, akin to how ramoops or other functionalities are represented. >> >> You don't need a binding for your case. Instantiate it whatever you wish >> - modprobe for example - and configure through approved kernel >> interfaces - sysfs for example. > > About using sysfs for the NVMEM cell, it won't work for my needs because > I have to know about reboot events before the filesystem is ready. So, initrd can configure it before mounting filesystem. > I'm thinking of using a boot parameter for the kernel. It would look > like this: pscrr-nvmem.nvmem_cell_alias=nvmem-cell0. This way, I can set > up the NVMEM cell right at the system's start. I'll need to use stable > NVMEM cell names for this. Is it ok to introduce NVMEM cell aliases in > the devicetree? In my opinion no, because the point of Devicetree is not to solve your system init problems. You add pure software node which should not be in your DTS for many reasons: it is not a hardware description and it is a software policy enforced on all users of DTS without actually consulting them. Also, this solution ignores ACPI systems. Developing a proper interface would work on ACPI as well. Best regards, Krzysztof
changes v2: - rename the framework from PSCR to PSCRR (last R is for Recorder) - extend power on reason header and use it to show detected reason on system start and in sysfs. - remove "unknow" reason - rebase on top of v6.8-rc1 - yaml fixes - zero reason state on boot 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 (8): power: Extend power_on_reason.h for upcoming PSCRR framework dt-bindings: power: reset: add generic PSCRR binding trackers power: reset: Introduce PSCR Recording 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 Recording Power State Change Reasons regulator: set Power State Change Reason before hw_protection_shutdown() thermal: core: Record PSCR before hw_protection_shutdown() .../bindings/power/reset/pscrr-nvmem.yaml | 53 +++ .../bindings/power/reset/pscrr.yaml | 44 +++ drivers/nvmem/core.c | 25 ++ drivers/power/reset/Kconfig | 30 ++ drivers/power/reset/Makefile | 2 + drivers/power/reset/pscrr-nvmem.c | 121 ++++++ drivers/power/reset/pscrr.c | 353 ++++++++++++++++++ drivers/regulator/core.c | 6 + drivers/thermal/thermal_core.c | 3 + include/linux/nvmem-consumer.h | 7 + include/linux/power/power_on_reason.h | 3 + include/linux/pscrr.h | 73 ++++ 12 files changed, 720 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/reset/pscrr-nvmem.yaml create mode 100644 Documentation/devicetree/bindings/power/reset/pscrr.yaml create mode 100644 drivers/power/reset/pscrr-nvmem.c create mode 100644 drivers/power/reset/pscrr.c create mode 100644 include/linux/pscrr.h