Re: [PATCH v3 2/3] power: reset: Add PSCRR NVMEM recorder

[Faruque Ansari]

Hi Srinivas,

On 22-Oct-26 1:24 PM, Srinivas Kandagatla wrote:
> On 9/14/26 5:38 AM, Faruque Ansari wrote:
> > Record the power state change reason into a NVMEM cell (a PMIC
> > SDAM byte, RTC scratch register, or EEPROM) and read it back on the
> > next boot, so the cause survives a power cycle.
> >
> > The recorder is implemented as an NVMEM layout driver: it owns the
> > single cell described by its layout binding and adds it to the
> > parent NVMEM device, rather than acting as a standalone consumer of
> > someone else's cell.
> >
> > Co-developed-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > Signed-off-by: Faruque Ansari <faruque.ansari@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/power/reset/pscrr/Kconfig       |  14 ++
> >   drivers/power/reset/pscrr/Makefile      |   1 +
> >   drivers/power/reset/pscrr/pscrr-nvmem.c | 269 ++++++++++++++++++++++++++++++++
>
> Consider moving this to drivers/nvmem, as this is an independent driver
> and no dependency to drivers/power headers.

Thanks for your review and suggestion.
Sure, I'll move this driver under drivers/nvmem/layout in the 
next revision.

> >   3 files changed, 284 insertions(+)
> >
> > diff --git a/drivers/power/reset/pscrr/Kconfig b/drivers/power/reset/pscrr/Kconfig
> > index 72de82731b53..459b05ba32d3 100644
> > --- a/drivers/power/reset/pscrr/Kconfig
> > +++ b/drivers/power/reset/pscrr/Kconfig
> > @@ -31,3 +31,17 @@ menuconfig PSCRR
> >   	  unless hardware provides the reset cause.
> >   
> >   	  If unsure, say N.
> > +
> > +if PSCRR
> > +
> > +config PSCRR_NVMEM
> > +	tristate "PSCRR NVMEM recorder provider"
> > +	depends on NVMEM_LAYOUTS
> > +	help
> > +	  PSCRR recorder that stores the power state change reason in a
> > +	  small NVMEM cell (such as an RTC scratch register) and reads it
> > +	  back on the next boot, so the cause survives a power cycle.
> > +
> > +	  If unsure, say N.
> > +
> > +endif # PSCRR
> > diff --git a/drivers/power/reset/pscrr/Makefile b/drivers/power/reset/pscrr/Makefile
> > index e5530a858971..95c80c80c7da 100644
> > --- a/drivers/power/reset/pscrr/Makefile
> > +++ b/drivers/power/reset/pscrr/Makefile
> > @@ -1,2 +1,3 @@
> >   # SPDX-License-Identifier: GPL-2.0
> >   obj-$(CONFIG_PSCRR) += pscrr.o
> > +obj-$(CONFIG_PSCRR_NVMEM) += pscrr-nvmem.o
> > diff --git a/drivers/power/reset/pscrr/pscrr-nvmem.c b/drivers/power/reset/pscrr/pscrr-nvmem.c
> > new file mode 100644
> > index 000000000000..64f67f0521e0
> > --- /dev/null
> > +++ b/drivers/power/reset/pscrr/pscrr-nvmem.c
> > @@ -0,0 +1,269 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * pscrr-nvmem.c - NVMEM layout driver for PSCRR
> > + *
> > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> > + * Copyright (C) 2025 Pengutronix, Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/nvmem-provider.h>
> > +#include <linux/of.h>
> > +#include <linux/pscrr.h>
> > +#include <linux/slab.h>
> > +
> > +struct pscrr_nvmem {
> > +	struct nvmem_device *nvmem;
> > +	u32 offset;		/* byte offset of the cell, from reg[0] */
> > +	u32 bit_offset;		/* 0 when the cell has no "bits" property */
> > +	u32 nbits;		/* BITS_PER_BYTE when the cell has no "bits" property */
> > +	enum psc_reason reason;
> > +};
> > +
> > +static inline u8 pscrr_nvmem_mask(struct pscrr_nvmem *priv)
> > +{
> > +	return (u8)GENMASK(priv->bit_offset + priv->nbits - 1, priv->bit_offset);
> > +}
> > +
> > +static int pscrr_nvmem_raw_read(struct pscrr_nvmem *priv, u8 *raw)
> > +{
> > +	int ret;
> > +
> > +	ret = nvmem_device_read(priv->nvmem, priv->offset, sizeof(*raw), raw);
> > +
> > +	return ret < 0 ? ret : 0;
> > +}
> > +
> > +static int pscrr_nvmem_raw_write(struct pscrr_nvmem *priv, u8 raw)
> > +{
> > +	int ret;
> > +
> > +	ret = nvmem_device_write(priv->nvmem, priv->offset, sizeof(raw), &raw);
> > +
> > +	return ret < 0 ? ret : 0;
> > +}
> > +
> > +static int pscrr_nvmem_cell_read(struct pscrr_nvmem *priv, u8 *out)
> > +{
> > +	u8 raw, mask = pscrr_nvmem_mask(priv);
> > +	int ret;
> > +
> > +	ret = pscrr_nvmem_raw_read(priv, &raw);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*out = (raw & mask) >> priv->bit_offset;
> > +
> > +	return 0;
> > +}
> > +
> > +static int pscrr_nvmem_cell_write(struct pscrr_nvmem *priv, u8 val)
> > +{
> > +	u8 raw, mask = pscrr_nvmem_mask(priv);
> > +	int ret;
> > +
> > +	/* Sub-byte cell: preserve the neighbouring bits, read-modify-write. */
> > +	if (priv->nbits < BITS_PER_BYTE) {
> > +		ret = pscrr_nvmem_raw_read(priv, &raw);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		raw = 0;
> > +	}
> > +
> > +	raw = (raw & ~mask) | ((val << priv->bit_offset) & mask);
> > +
> > +	return pscrr_nvmem_raw_write(priv, raw);
> > +}
> > +
> > +static int pscrr_nvmem_read_reasons(struct pscrr_provider *p,
> > +				    unsigned long *reasons)
> > +{
> > +	struct pscrr_nvmem *priv = p->priv;
> > +
> > +	/*
> > +	 * Report the reason latched at probe (or overwritten by a later
> > +	 * record); the cell itself was cleared at probe, so it is not read
> > +	 * live here.
> > +	 */
> > +	set_bit(READ_ONCE(priv->reason), reasons);
> > +
> > +	return 0;
> > +}
> > +
> > +static int pscrr_nvmem_write_reason(struct pscrr_provider *p,
> > +				    enum psc_reason reason)
> > +{
> > +	struct pscrr_nvmem *priv = p->priv;
> > +	int ret;
> > +
> > +	if (reason >= PSCR_REASON_COUNT)
> > +		return -EINVAL;
> > +
> > +	ret = pscrr_nvmem_cell_write(priv, reason);
> > +	if (ret)
> > +		return ret;
> > +
> > +	WRITE_ONCE(priv->reason, reason);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pscrr_provider_ops pscrr_nvmem_ops = {
> > +	.read_reasons = pscrr_nvmem_read_reasons,
> > +	.write_reason = pscrr_nvmem_write_reason,
> > +};
> > +
> > +static struct device_node *pscrr_nvmem_get_cell(struct device *dev,
> > +						struct nvmem_device *nvmem,
> > +						u32 reg[2])
> > +{
> > +	struct device_node *layout_np, *cell_np = NULL;
> > +
> > +	layout_np = of_nvmem_layout_get_container(nvmem);
> > +	if (!layout_np)
> > +		return ERR_PTR(-ENOENT);
> > +
> > +	for_each_child_of_node_scoped(layout_np, child) {
> > +		if (of_property_read_u32_array(child, "reg", reg, 2))
> > +			continue;
> Please explain this, why are you ignoring child with reg? to start with
> are you expecting child nodes without reg property?

ACK.
Yes. Child nodes without a reg property are expected when the 
complete byte is reserved for a single cell. If only a portion of 
the byte is being used (e.g. 4 bits out of 8 bits), then the 
child node should provide a reg property to describe the bit range.>
> > +
> > +		cell_np = of_node_get(child);
> You are getting only the first child? Please explain why this is not
> inline with the bindings or vice-versa.

ACK.
Agree — silently taking the first child was wrong. PSCRR records 
one reason into one cell, and the binding describes exactly one 
reason cell.
I'll fix this so the driver returns an error in both out-of-spec 
cases instead of guessing:
- more than one cell  -> -EINVAL ("expected a single pscr nvmem 
cell")
- zero cells          -> -ENOENT ("missing pscr nvmem cell")


> > +		break;
> > +	}
> > +	of_node_put(layout_np);
> > +
> > +	return cell_np ?: ERR_PTR(dev_err_probe(dev, -ENOENT,
> > +						"missing pscr nvmem cell\n"));
> > +}
> > +
> > +static int pscrr_nvmem_add_cells(struct nvmem_layout *layout)
> > +{
> > +	struct device *dev = &layout->dev;
> > +	struct pscrr_nvmem *priv;
> > +	struct device_node *cell_np;
> > +	struct nvmem_cell_info info = { };
> > +	u32 reg[2], bits[2];
> > +	u8 val;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->nvmem = layout->nvmem;
> > +
> > +	cell_np = pscrr_nvmem_get_cell(dev, priv->nvmem, reg);
> > +	if (IS_ERR(cell_np))
> > +		return PTR_ERR(cell_np);
> > +
> > +	/* The reason is a single byte; a bit cell still occupies one. */
> > +	if (reg[1] != sizeof(val)) {
> > +		of_node_put(cell_np);
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "unsupported pscr nvmem cell size\n");
> 6 such instances with same exit point, consider using label.

ACK. I'll address this in the next revision.
> > +	}
> > +	priv->offset = reg[0];
> > +
> > +	if (!of_property_read_u32_array(cell_np, "bits", bits, ARRAY_SIZE(bits))) {
> > +		priv->bit_offset = bits[0];
> > +		priv->nbits = bits[1];
> > +	} else {
> > +		priv->bit_offset = 0;
> > +		priv->nbits = BITS_PER_BYTE;
> > +	}
> > +
> > +	if (priv->bit_offset + priv->nbits > BITS_PER_BYTE * reg[1]) {
> > +		of_node_put(cell_np);
> > +		return dev_err_probe(dev, -EINVAL, "bits exceed cell size\n");
> > +	}
> > +
> > +	/* PSCR_REASON_COUNT must fit in the allotted bits. */
> > +	if (PSCR_REASON_COUNT > (1U << priv->nbits)) {
> > +		of_node_put(cell_np);
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "too few bits for the reason range\n");
> > +	}
> > +
> > +	/*
> > +	 * The cell survives resets, so at boot it still holds whatever the
> > +	 * previous session recorded before it went down. Latch that as this
> > +	 * boot's reason, then clear the cell back to PSCR_UNKNOWN: if this
> > +	 * session is later cut short by an abrupt reset that never runs the
> > +	 * recorder (watchdog power-cycle, sudden power loss), the next boot
> > +	 * reads "unknown" instead of a stale reason left over from an earlier
> > +	 * cycle.
> > +	 */
> Looks unnecessary comment tbh, has no value to the user.
ACK. I will remove this in the next revision.>
> > +	ret = pscrr_nvmem_cell_read(priv, &val);
> > +	if (ret) {
> > +		of_node_put(cell_np);
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to read the pscr nvmem cell\n");
> > +	}
> > +
> > +	priv->reason = val < PSCR_REASON_COUNT ? val : PSCR_UNKNOWN;
> > +
> > +	ret = pscrr_nvmem_cell_write(priv, PSCR_UNKNOWN);
> > +	if (ret) {
> > +		of_node_put(cell_np);
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to clear the pscr nvmem cell\n");
> > +	}
> > +
> > +	info.name = kasprintf(GFP_KERNEL, "%pOFn", cell_np);
> > +	if (!info.name) {
> > +		of_node_put(cell_np);
> > +		return -ENOMEM;
> > +	}
> > +	info.offset = reg[0];
> > +	info.bytes = reg[1];
> > +	info.bit_offset = priv->bit_offset;
> > +	info.nbits = priv->nbits;
> > +	info.np = cell_np;
> > +
> > +	ret = nvmem_add_one_cell(priv->nvmem, &info);
> > +	kfree(info.name);
> > +	if (ret) {
> > +		of_node_put(cell_np);
> > +		return ret;
> > +	}
> > +
> > +	return PTR_ERR_OR_ZERO(devm_pscrr_provider_register(dev, "nvmem",
> > +							    &pscrr_nvmem_ops, NULL,
> > +							    priv));
> > +}
> > +
> > +static int pscrr_nvmem_probe(struct nvmem_layout *layout)
> > +{
> > +	layout->add_cells = pscrr_nvmem_add_cells;
> > +
> > +	return nvmem_layout_register(layout);
> > +}
> > +
> > +static void pscrr_nvmem_remove(struct nvmem_layout *layout)
> > +{
> > +	nvmem_layout_unregister(layout);
> > +}
> > +
> > +static const struct of_device_id pscrr_nvmem_of_match[] = {
> > +	{ .compatible = "pscrr-nvmem" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, pscrr_nvmem_of_match);
> > +
> > +static struct nvmem_layout_driver pscrr_nvmem_layout = {
> > +	.driver	= {
> > +		.name		= "pscrr-nvmem",
> > +		.of_match_table	= pscrr_nvmem_of_match,
> > +	},
> > +	.probe	= pscrr_nvmem_probe,
> > +	.remove	= pscrr_nvmem_remove,
> > +};
> > +module_nvmem_layout_driver(pscrr_nvmem_layout);
> > +
> > +MODULE_AUTHOR("Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Faruque Ansari <faruque.ansari@xxxxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("NVMEM recorder provider for PSCRR");
> > +MODULE_LICENSE("GPL");