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
--
2.39.2
This driver utilizes the Power State Change Reasons Recording (PSCRR)
framework to store specific power state change information, such as
shutdown or reboot reasons, into a designated non-volatile memory
(NVMEM) cell.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/power/reset/Kconfig | 11 +++
drivers/power/reset/Makefile | 1 +
drivers/power/reset/pscrr-nvmem.c | 121 ++++++++++++++++++++++++++++++
3 files changed, 133 insertions(+)
create mode 100644 drivers/power/reset/pscrr-nvmem.c
diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index c6ce7e647048..8e50e495ef98 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -324,3 +324,14 @@ menuconfig PSCRR
timeouts.
If unsure, say N.
+
+if PSCRR
+
+config PSCRR_NVMEM
+ tristate "Generic NVMEM-based Power State Change Reason Recorder"
+ depends on OF
+ help
+ Enabling this option adds support for recording power state change
+ reasons in a NVMEM cell.
+
+endif
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index e618c34a30f9..3860dbbacb23 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -32,6 +32,7 @@ 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_PSCRR_NVMEM) += pscrr-nvmem.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-nvmem.c b/drivers/power/reset/pscrr-nvmem.c
new file mode 100644
index 000000000000..fe9053b78b79
--- /dev/null
+++ b/drivers/power/reset/pscrr-nvmem.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) Vaisala Oyj. All rights reserved.
+// Copyright (c) 2024 Pengutronix, Oleksij Rempel <[email protected]>
+/*
+ * Based on drivers/power/reset/nvmem-reboot-mode.c
+ * Copyright (c) Vaisala Oyj. All rights reserved.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pscrr.h>
+
+struct pscrr_nvmem {
+ struct pscrr_device pscrr_dev;
+ struct nvmem_cell *cell;
+ size_t max_magic_bytes;
+};
+
+static int pscrr_nvmem_write(struct pscrr_device *pscrr_dev, u32 magic)
+{
+ struct pscrr_nvmem *priv = container_of(pscrr_dev, struct pscrr_nvmem,
+ pscrr_dev);
+ size_t size = min(priv->max_magic_bytes, sizeof(magic));
+ int ret;
+
+ ret = nvmem_cell_write(priv->cell, &magic, size);
+ if (ret < 0) {
+ dev_err(pscrr_dev->dev, "update reason bits failed: %pe\n",
+ ERR_PTR(ret));
+ return ret;
+ }
+
+ return 0;
+}
+
+static int pscrr_nvmem_read(struct pscrr_device *pscrr_dev, u32 *magic)
+{
+ struct pscrr_nvmem *priv = container_of(pscrr_dev, struct pscrr_nvmem,
+ pscrr_dev);
+ size_t len;
+ void *buf;
+
+ buf = nvmem_cell_read(priv->cell, &len);
+ if (IS_ERR(buf))
+ return PTR_ERR(buf);
+
+ *magic = 0;
+ memcpy(magic, buf, min(len, sizeof(*magic)));
+ kfree(buf);
+
+ return 0;
+}
+
+static int pscrr_nvmem_probe(struct platform_device *pdev)
+{
+ size_t bytes, bits, magic_bits;
+ struct pscrr_nvmem *priv;
+ const char *pscr = "pscr";
+ int ret;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->pscrr_dev.dev = &pdev->dev;
+ priv->pscrr_dev.write = pscrr_nvmem_write;
+ priv->pscrr_dev.read = pscrr_nvmem_read;
+
+ priv->cell = devm_nvmem_cell_get(&pdev->dev, pscr);
+ if (IS_ERR(priv->cell))
+ return dev_err_probe(&pdev->dev, PTR_ERR(priv->cell),
+ "failed to get the nvmem %s cell\n", pscr);
+
+ ret = nvmem_cell_get_size(priv->cell, &bytes, &bits);
+ if (ret < 0)
+ return dev_err_probe(&pdev->dev, ret, "failed to get the nvmem %s size\n",
+ pscr);
+
+ if (!bytes || bytes > sizeof(u32) || bits > 32)
+ return dev_err_probe(&pdev->dev, -EINVAL, "invalid nvmem %s size. bytes: %zu, bits: %zu\n",
+ pscr, bytes, bits);
+
+ ret = devm_pscrr_register(&pdev->dev, &priv->pscrr_dev);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret, "failed to register pscr driver\n");
+
+ magic_bits = fls(priv->pscrr_dev.max_magic_val);
+ priv->max_magic_bytes = DIV_ROUND_UP(magic_bits, 8);
+
+ if (!bits)
+ bits = bytes * 8;
+
+ if (magic_bits > bits)
+ return dev_err_probe(&pdev->dev, -EINVAL, "provided magic can't fit into nvmem %s. bytes: %zu, bits: %zu, magic_bits: %zu\n",
+ pscr, bytes, bits, magic_bits);
+
+ return handle_last_pscr(&priv->pscrr_dev);
+}
+
+static const struct of_device_id pscrr_nvmem_of_match[] = {
+ { .compatible = "pscrr-nvmem" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, pscrr_nvmem_of_match);
+
+static struct platform_driver pscrr_nvmem_driver = {
+ .probe = pscrr_nvmem_probe,
+ .driver = {
+ .name = "pscrr-nvmem",
+ .of_match_table = pscrr_nvmem_of_match,
+ },
+};
+module_platform_driver(pscrr_nvmem_driver);
+
+MODULE_AUTHOR("Oleksij Rempel <[email protected]>");
+MODULE_DESCRIPTION("NVMEM Driver for Power State Change Reason Recording");
+MODULE_LICENSE("GPL");
--
2.39.2
Store the state change reason to some black box, for later
investigation.
Signed-off-by: Oleksij Rempel <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
---
drivers/regulator/core.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index a968dabb48f5..a811a5ff2273 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -19,6 +19,7 @@
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
#include <linux/of.h>
+#include <linux/pscrr.h>
#include <linux/reboot.h>
#include <linux/regmap.h>
#include <linux/regulator/of_regulator.h>
@@ -5095,6 +5096,7 @@ EXPORT_SYMBOL_GPL(regulator_bulk_free);
static void regulator_handle_critical(struct regulator_dev *rdev,
unsigned long event)
{
+ enum pscr pscr;
const char *reason = NULL;
if (!rdev->constraints->system_critical)
@@ -5103,17 +5105,21 @@ static void regulator_handle_critical(struct regulator_dev *rdev,
switch (event) {
case REGULATOR_EVENT_UNDER_VOLTAGE:
reason = "System critical regulator: voltage drop detected";
+ pscr = PSCR_UNDER_VOLTAGE;
break;
case REGULATOR_EVENT_OVER_CURRENT:
reason = "System critical regulator: over-current detected";
+ pscr = PSCR_OVER_CURRENT;
break;
case REGULATOR_EVENT_FAIL:
reason = "System critical regulator: unknown error";
+ pscr = PSCR_REGULATOR_FAILURE;
}
if (!reason)
return;
+ set_power_state_change_reason(pscr);
hw_protection_shutdown(reason,
rdev->constraints->uv_less_critical_window_ms);
}
--
2.39.2
Add binding for Power State Change Reason Recording (PSCRR) subsystem
Signed-off-by: Oleksij Rempel <[email protected]>
---
.../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)
+
+maintainers:
+ - Oleksij Rempel <[email protected]>
+
+description: Binding for devices responsible to store reasons for power state
+ 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.
+
+ pscr-over-current:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ Value to indicate an over-current condition of a system ctitical regulator
+ as the reason for the power state change.
+
+ pscr-regulator-failure:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ Value to indicate an unknown, system ctitical regulator related failure
+ as the reason for the power state change.
+
+ pscr-over-temperature:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ Value to indicate a system critical over-temperature condition as the
+ reason for the power state change.
+
+additionalProperties: true
+
+...
--
2.39.2
Add nvmem_cell_get_size() function to provide access to cell size
metrics. In some cases we may get cell size less as consumer would
expect it. So, nvmem_cell_write() would fail with incorrect buffer size.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/nvmem/core.c | 25 +++++++++++++++++++++++++
include/linux/nvmem-consumer.h | 7 +++++++
2 files changed, 32 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 980123fb4dde..a21e5649fcda 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1790,6 +1790,31 @@ int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len)
EXPORT_SYMBOL_GPL(nvmem_cell_write);
+/**
+ * nvmem_cell_get_size() - Get the size of a given nvmem cell
+ * @cell: nvmem cell to be queried.
+ * @bytes: Pointer to store the size of the cell in bytes. Can be NULL.
+ * @bits: Pointer to store the size of the cell in bits. Can be NULL.
+ *
+ * Return: 0 on success or negative on failure.
+ */
+int nvmem_cell_get_size(struct nvmem_cell *cell, size_t *bytes, size_t *bits)
+{
+ struct nvmem_cell_entry *entry = cell->entry;
+
+ if (!entry->nvmem)
+ return -EINVAL;
+
+ if (bytes)
+ *bytes = entry->bytes;
+
+ if (bits)
+ *bits = entry->nbits;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(nvmem_cell_get_size);
+
static int nvmem_cell_read_common(struct device *dev, const char *cell_id,
void *val, size_t count)
{
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 34c0e58dfa26..bcb0e17e415d 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -56,6 +56,7 @@ void nvmem_cell_put(struct nvmem_cell *cell);
void devm_nvmem_cell_put(struct device *dev, struct nvmem_cell *cell);
void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len);
int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len);
+int nvmem_cell_get_size(struct nvmem_cell *cell, size_t *bytes, size_t *bits);
int nvmem_cell_read_u8(struct device *dev, const char *cell_id, u8 *val);
int nvmem_cell_read_u16(struct device *dev, const char *cell_id, u16 *val);
int nvmem_cell_read_u32(struct device *dev, const char *cell_id, u32 *val);
@@ -128,6 +129,12 @@ static inline int nvmem_cell_write(struct nvmem_cell *cell,
return -EOPNOTSUPP;
}
+static inline int nvmem_cell_get_size(struct nvmem_cell *cell, size_t *bytes,
+ size_t *bits)
+{
+ return -EOPNOTSUPP;
+}
+
static inline int nvmem_cell_read_u8(struct device *dev,
const char *cell_id, u8 *val)
{
--
2.39.2
Add device tree bindings that describe hardware implementations of
Non-Volatile Memory (NVMEM) used for storing Power State Change Reasons
(PSCR).
Signed-off-by: Oleksij Rempel <[email protected]>
---
.../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 <[email protected]>
+
+description: This binding describes the Non-Volatile Memory (NVMEM) hardware
+ that stores Power State Change Reasons (PSCR).
+
+allOf:
+ - $ref: pscrr.yaml#
+
+properties:
+ compatible:
+ const: pscrr-nvmem
+
+ nvmem-cells:
+ description: |
+ A phandle pointing to the nvmem-cells node where the power state change
+ reasons are stored.
+ maxItems: 1
+
+ nvmem-cell-names:
+ items:
+ - const: pscr
+
+ pscr-under-voltage: true
+ pscr-over-current: true
+ pscr-regulator-failure: true
+ pscr-over-temperature: true
+
+required:
+ - compatible
+ - nvmem-cells
+ - nvmem-cell-names
+
+additionalProperties: false
+
+examples:
+ - |
+ power-state-change-reason {
+ compatible = "pscrr-nvmem";
+ nvmem-cells = <&pscr_cell>;
+ nvmem-cell-names = "pscr";
+ pscr-under-voltage = <1>;
+ pscr-over-temperature = <2>;
+ };
+...
--
2.39.2
On 24/01/2024 13:21, Oleksij Rempel wrote:
> Add binding for Power State Change Reason Recording (PSCRR) subsystem
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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 <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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
--
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 |
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 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,
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?
Best 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 |
Hi Oleksij,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on broonie-regulator/for-next rafael-pm/thermal linus/master v6.8-rc1 next-20240125]
[cannot apply to sre-power-supply/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Oleksij-Rempel/power-Extend-power_on_reason-h-for-upcoming-PSCRR-framework/20240124-202833
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240124122204.730370-7-o.rempel%40pengutronix.de
patch subject: [PATCH v2 6/8] power: reset: add PSCR NVMEM Driver for Recording Power State Change Reasons
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20240128/[email protected]/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240128/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> drivers/power/reset/pscrr-nvmem.c:53:2: error: call to undeclared function 'kfree'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
53 | kfree(buf);
| ^
1 error generated.
vim +/kfree +53 drivers/power/reset/pscrr-nvmem.c
39
40 static int pscrr_nvmem_read(struct pscrr_device *pscrr_dev, u32 *magic)
41 {
42 struct pscrr_nvmem *priv = container_of(pscrr_dev, struct pscrr_nvmem,
43 pscrr_dev);
44 size_t len;
45 void *buf;
46
47 buf = nvmem_cell_read(priv->cell, &len);
48 if (IS_ERR(buf))
49 return PTR_ERR(buf);
50
51 *magic = 0;
52 memcpy(magic, buf, min(len, sizeof(*magic)));
> 53 kfree(buf);
54
55 return 0;
56 }
57
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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