[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <58e19481-530c-4465-aec5-1f44462eaf5f@gmail.com>
Date: Fri, 14 Mar 2025 14:22:40 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>,
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>
Cc: kernel@...gutronix.de, linux-kernel@...r.kernel.org,
Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, Zhang Rui <rui.zhang@...el.com>,
Lukasz Luba <lukasz.luba@....com>, linux-pm@...r.kernel.org,
Søren Andersen <san@...v.dk>,
Guenter Roeck <groeck@...omium.org>, Ahmad Fatoum <a.fatoum@...gutronix.de>,
Andrew Morton <akpm@...ux-foundation.org>, chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v6 3/7] power: reset: Introduce PSCR Recording Framework
for Non-Volatile Storage
Hi deee Ho Oleksij,
On 14/03/2025 13:36, 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.
>
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
I see you're already at v6, so I am probably slightly late... I think I
hadn't noticed this before. Thus, feel free to treat my comments as mere
suggestions.
All in all, I do like this series. Looks mostly very good to me :) Just
wondering if we could utilize this same for standardizing reading the
reset reason registers which are included in many PMICs?
> ---
...
> +int pscrr_core_init(const struct pscrr_backend_ops *ops)
> +{
> + enum psc_reason stored_val = PSCR_UNKNOWN;
> + int ret;
> +
> + mutex_lock(&pscrr_lock);
> +
> + if (g_pscrr) {
> + pr_err("PSCRR: Core is already initialized!\n");
> + ret = -EBUSY;
> + goto err_unlock;
> + }
> +
> + if (!ops->read_reason || !ops->write_reason) {
> + pr_err("PSCRR: Backend must provide read and write callbacks\n");
Why both are required?
I can easily envision integrating the some PMIC's 'boot reason' register
reading to the PSCRR. Benefit would be that user-space could use this
same interface when reading the reset reason on a system where reason is
stored using mechanisms provided by this series - and when reset reason
is automatically stored by the HW (for example to a PMIC).
In a PMIC case the write_reason might not be needed, right?
> + ret = -EINVAL;
> + goto err_unlock;
> + }
> +
> + g_pscrr = kzalloc(sizeof(*g_pscrr), GFP_KERNEL);
> + if (!g_pscrr) {
> + ret = -ENOMEM;
> + goto err_unlock;
> + }
> +
> + g_pscrr->ops = ops;
> + g_pscrr->last_boot_reason = PSCR_UNKNOWN;
> +
> + ret = ops->read_reason(&stored_val);
> + if (!ret) {
> + g_pscrr->last_boot_reason = stored_val;
> + pr_info("PSCRR: Initial read_reason: %d (%s)\n",
> + stored_val, psc_reason_to_str(stored_val));
> + } else {
> + pr_warn("PSCRR: read_reason failed, err=%pe\n",
> + ERR_PTR(ret));
> + }
...
> +/**
> + * struct pscrr_backend_ops - Backend operations for storing power state change
> + * reasons.
> + *
> + * This structure defines the interface for backend implementations that handle
> + * the persistent storage of power state change reasons. Different backends
> + * (e.g., NVMEM, EEPROM, battery-backed RAM) can implement these operations to
> + * store and retrieve shutdown reasons across reboots.
Maybe we should support / mention also a case where the PMIC driver
could just register the read-callback and provide the reset reason
stored by the PMIC to users. In this case the write_reason might not be
needed.
> + *
> + * @write_reason: Function pointer to store the specified `psc_reason` in
> + * persistent storage. This function is called before a reboot
> + * to record the last power state change reason.
> + * @read_reason: Function pointer to retrieve the last stored `psc_reason`
> + * from persistent storage. This function is called at boot to
> + * restore the shutdown reason.
> + */
> +struct pscrr_backend_ops {
> + int (*write_reason)(enum psc_reason reason);
> + int (*read_reason)(enum psc_reason *reason);
> +};
Yours,
-- Matti
Powered by blists - more mailing lists