[PATCH v2 2/6] misc: Add support for bit fields in NVMEM cells
Sean Anderson
seanga2 at gmail.com
Fri Feb 13 16:34:33 CET 2026
On 2/13/26 06:27, Aswin Murugan wrote:
> NVMEM cells currently only support byte-level access. Many hardware
> registers pack multiple fields into single bytes, requiring bit-level
> granularity. For example, Qualcomm PMIC PON registers store a 7-bit
> reboot reason field within a single byte, with bit 0 reserved for other
> purposes.
>
> Add support for the optional 'bits' property in NVMEM cell device tree
> bindings. This property specifies <bit_offset num_bits> to define a bit
> field within the cell's register space.
>
> Implement multi-byte bit field support by porting bit manipulation
> algorithms from the Linux kernel driver [1]:
>
> 1. nvmem_shift_read_buffer_in_place() - Extract bit fields from raw
> bytes by shifting and masking across byte boundaries. Handles fields
> that span multiple bytes.
>
> 2. nvmem_cell_prepare_write_buffer() - Perform Read-Modify-Write to
> preserve bits outside the target field. Read current values, mask to
> preserve adjacent bits, and combine with new value.
>
> Example device tree usage:
> reboot-reason at 48 {
> reg = <0x48 0x01>;
> bits = <0x01 0x07>; /* 7 bits starting at bit 1 */
> };
>
> This reads bits [7:1] from the byte at offset 0x48, leaving bit 0
> untouched during write operations.
>
> Cells without the 'bits' property continue to work unchanged, ensuring
> backward compatibility with existing device trees.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvmem/core.c
>
> Signed-off-by: Aswin Murugan <aswin.murugan at oss.qualcomm.com>
This seems like it adds a lot of calculations; have you looked at the size
increase? Maybe this should be a separate config, since the feature does not
seem too common.
Actually, I think we could reduce the complexity quite a bit by refusing
to process bits longer than a ulong. A quick grep through Linux's dts shows
that the largest bits is <6 12>. I don't really see any reason we need to
support arbitrary bit extraction.
There should also be a test for this. The existing code is lightly tested by
checking that mac addresses are loaded correctly. Maybe you can extend
test/dm/reboot-mode.c.
> ---
> drivers/misc/nvmem.c | 241 ++++++++++++++++++++++++++++++++++++++++---
> include/nvmem.h | 4 +
> 2 files changed, 231 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/misc/nvmem.c b/drivers/misc/nvmem.c
> index 33e80858565..6a75c326189 100644
> --- a/drivers/misc/nvmem.c
> +++ b/drivers/misc/nvmem.c
> @@ -12,52 +12,251 @@
> #include <dm/ofnode.h>
> #include <dm/read.h>
> #include <dm/uclass.h>
> +#include <linux/bitops.h>
> +#include <linux/kernel.h>
> +
> +/**
> + * nvmem_shift_read_buffer_in_place() - Shift read buffer to extract bit field
> + * @cell: NVMEM cell with bit field information
> + * @buf: Buffer containing raw bytes read from NVMEM
> + *
> + * This function shifts and masks the buffer to extract only the bits specified
> + * by cell->bit_offset and cell->nbits. It handles bit fields that span multiple
> + * bytes by shifting bits across byte boundaries.
> + *
> + */
> +static void nvmem_shift_read_buffer_in_place(struct nvmem_cell *cell, void *buf)
> +{
> + u8 *prev_byte, *buf_ptr;
> + int i, extra, bit_offset = cell->bit_offset;
> + int bytes_needed;
> +
> + bytes_needed = DIV_ROUND_UP(cell->nbits + cell->bit_offset, BITS_PER_BYTE);
> +
> + prev_byte = buf_ptr = buf;
> + if (bit_offset) {
> + /* First byte: shift right by bit_offset */
> + *buf_ptr++ >>= bit_offset;
> +
> + /* Process remaining bytes, combining bits across boundaries */
> + for (i = 1; i < bytes_needed; i++) {
> + /* Get bits from next byte and shift them towards MSB */
> + *prev_byte |= *buf_ptr << (BITS_PER_BYTE - bit_offset);
> +
> + prev_byte = buf_ptr;
> + *buf_ptr++ >>= bit_offset;
> + }
> + } else {
> + /* Point to the MSB */
> + prev_byte += bytes_needed - 1;
> + }
> +
> + /* Clear extra bytes if result fits in fewer bytes */
> + extra = bytes_needed - DIV_ROUND_UP(cell->nbits, BITS_PER_BYTE);
> + while (--extra >= 0)
> + *prev_byte-- = 0;
> +
> + /* Clear MSB bits if any leftover in the last byte */
> + if (cell->nbits % BITS_PER_BYTE)
> + *prev_byte &= GENMASK((cell->nbits % BITS_PER_BYTE) - 1, 0);
> +}
> +
> +/**
> + * nvmem_cell_prepare_write_buffer() - Prepare buffer for writing bit field
> + * @cell: NVMEM cell with bit field information
> + * @buf: Buffer containing value to write
> + * @size: Size of buf
> + * @data: Output buffer with prepared data for writing
> + *
> + * This function performs Read-Modify-Write to preserve bits outside the
> + * specified bit field. It reads current values, modifies only the target
> + * bits, and prepares the complete bytes for writing back.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
> + const void *buf, size_t size,
> + u8 *data)
> +{
> + int i, rc, nbits, bit_offset = cell->bit_offset;
> + int bytes_needed;
> + u8 current_val, *prev_byte, *buf_ptr, prev_byte_val, prev_bits;
> +
> + nbits = cell->nbits;
> + bytes_needed = DIV_ROUND_UP(nbits + bit_offset, BITS_PER_BYTE);
> +
> + memcpy(data, buf, size);
> + prev_byte = buf_ptr = data;
> +
> + if (bit_offset) {
> + prev_byte_val = *buf_ptr;
> + *buf_ptr <<= bit_offset;
> +
> + /* Setup the first byte with LSB bits from NVMEM */
> + switch (cell->nvmem->driver->id) {
> + case UCLASS_I2C_EEPROM:
> + rc = i2c_eeprom_read(cell->nvmem, cell->offset, ¤t_val, 1);
> + break;
> + case UCLASS_MISC:
> + rc = misc_read(cell->nvmem, cell->offset, ¤t_val, 1);
> + if (rc < 0)
> + return rc;
> + if (rc != 1)
> + return -EIO;
> + rc = 0;
> + break;
> + case UCLASS_RTC:
> + rc = dm_rtc_read(cell->nvmem, cell->offset, ¤t_val, 1);
> + break;
> + default:
> + return -ENOSYS;
> + }
Can you use nvmem_cell_read for this to reduce duplication?
> +
> + if (rc)
> + return rc;
> +
> + *buf_ptr++ |= GENMASK(bit_offset - 1, 0) & current_val;
> +
> + /* Setup rest of the bytes if any */
> + for (i = 1; i < bytes_needed; i++) {
> + /* Get last byte bits and shift them towards LSB */
> + prev_bits = prev_byte_val >> (BITS_PER_BYTE - 1 - bit_offset);
> + prev_byte_val = *buf_ptr;
> + prev_byte = buf_ptr;
> + *buf_ptr <<= bit_offset;
> + *buf_ptr++ |= prev_bits;
> + }
> + }
> +
> + /* If it's not end on byte boundary */
> + if ((nbits + bit_offset) % BITS_PER_BYTE) {
> + /* Setup the last byte with MSB bits from NVMEM */
> + switch (cell->nvmem->driver->id) {
> + case UCLASS_I2C_EEPROM:
> + rc = i2c_eeprom_read(cell->nvmem,
> + cell->offset + bytes_needed - 1,
> + ¤t_val, 1);
> + break;
> + case UCLASS_MISC:
> + rc = misc_read(cell->nvmem,
> + cell->offset + bytes_needed - 1,
> + ¤t_val, 1);
> + if (rc < 0)
> + return rc;
> + if (rc != 1)
> + return -EIO;
> + rc = 0;
> + break;
> + case UCLASS_RTC:
> + rc = dm_rtc_read(cell->nvmem,
> + cell->offset + bytes_needed - 1,
> + ¤t_val, 1);
> + break;
> + default:
> + return -ENOSYS;
> + }
> +
> + if (rc)
> + return rc;
> +
> + *prev_byte |= GENMASK(7, (nbits + bit_offset) % BITS_PER_BYTE) & current_val;
> + }
> +
> + return 0;
> +}
>
> int nvmem_cell_read(struct nvmem_cell *cell, void *buf, size_t size)
> {
> + u8 data[size];
No alloca.
> + int bytes_needed;
> + int ret;
> +
> dev_dbg(cell->nvmem, "%s: off=%u size=%zu\n", __func__, cell->offset, size);
Please update these.
> +
> if (size != cell->size)
> return -EINVAL;
>
> + /* Calculate how many bytes we need to read */
> + if (cell->nbits > 0) {
just if (cell->nbits) for clarity please
(ditto for elsewhere)
> + bytes_needed = DIV_ROUND_UP(cell->nbits + cell->bit_offset, BITS_PER_BYTE);
> + if (bytes_needed > sizeof(data))
> + return -EINVAL;
> + } else {
> + bytes_needed = size;
This should always be true even when nbits != 0, as bits specifies the offset/size
"within the address range specified by reg." This will allow you to use the original buffer.
> + }
> +
> switch (cell->nvmem->driver->id) {
> case UCLASS_I2C_EEPROM:
> - return i2c_eeprom_read(cell->nvmem, cell->offset, buf, size);
> - case UCLASS_MISC: {
> - int ret = misc_read(cell->nvmem, cell->offset, buf, size);
> -
> + ret = i2c_eeprom_read(cell->nvmem, cell->offset, data, bytes_needed);
> + break;
> + case UCLASS_MISC:
> + ret = misc_read(cell->nvmem, cell->offset, data, bytes_needed);
> if (ret < 0)
> return ret;
> - if (ret != size)
> + if (ret != bytes_needed)
> return -EIO;
> - return 0;
> - }
> + ret = 0;
> + break;
> case UCLASS_RTC:
> - return dm_rtc_read(cell->nvmem, cell->offset, buf, size);
> + ret = dm_rtc_read(cell->nvmem, cell->offset, data, bytes_needed);
> + break;
> default:
> return -ENOSYS;
> }
> +
> + if (ret)
> + return ret;
> +
> + if (cell->nbits > 0) {
> + /* Extract bit field from raw bytes */
> + nvmem_shift_read_buffer_in_place(cell, data);
> + /* Copy only the bytes containing the bit field result */
> + memcpy(buf, data, DIV_ROUND_UP(cell->nbits, BITS_PER_BYTE));
> + } else {
> + memcpy(buf, data, size);
> + }
> +
> + return 0;
> }
>
> int nvmem_cell_write(struct nvmem_cell *cell, const void *buf, size_t size)
> {
> + u8 data[size];
Again, no alloca. Limit the size for cells with bits and you can use a static
allocation on the stack.
> + int bytes_needed;
> + int ret;
> +
> dev_dbg(cell->nvmem, "%s: off=%u size=%zu\n", __func__, cell->offset, size);
> +
> if (size != cell->size)
> return -EINVAL;
>
> + if (cell->nbits > 0) {
> + bytes_needed = DIV_ROUND_UP(cell->nbits + cell->bit_offset, BITS_PER_BYTE);
> + if (bytes_needed > sizeof(data))
> + return -EINVAL;
> +
> + /* Prepare write buffer with Read-Modify-Write */
> + ret = nvmem_cell_prepare_write_buffer(cell, buf, size, data);
> + if (ret)
> + return ret;
> + } else {
> + bytes_needed = size;
> + memcpy(data, buf, size);
> + }
> +
> switch (cell->nvmem->driver->id) {
> case UCLASS_I2C_EEPROM:
> - return i2c_eeprom_write(cell->nvmem, cell->offset, buf, size);
> + return i2c_eeprom_write(cell->nvmem, cell->offset, data, bytes_needed);
> case UCLASS_MISC: {
> - int ret = misc_write(cell->nvmem, cell->offset, buf, size);
> -
> + ret = misc_write(cell->nvmem, cell->offset, data, bytes_needed);
> if (ret < 0)
> return ret;
> - if (ret != size)
> + if (ret != bytes_needed)
> return -EIO;
> return 0;
> }
> case UCLASS_RTC:
> - return dm_rtc_write(cell->nvmem, cell->offset, buf, size);
> + return dm_rtc_write(cell->nvmem, cell->offset, data, bytes_needed);
> default:
> return -ENOSYS;
> }
> @@ -121,13 +320,27 @@ int nvmem_cell_get_by_index(struct udevice *dev, int index,
>
> offset = ofnode_get_addr_size_index_notrans(args.node, 0, &size);
> if (offset == FDT_ADDR_T_NONE || size == FDT_SIZE_T_NONE) {
> - dev_dbg(cell->nvmem, "missing address or size for %s\n",
> + dev_err(cell->nvmem, "missing address or size for %s\n",
> ofnode_get_name(args.node));
> return -EINVAL;
> }
>
> cell->offset = offset;
> cell->size = size;
> +
> + ret = ofnode_read_u32_index(args.node, "bits", 0, &cell->bit_offset);
> + if (ret) {
> + cell->bit_offset = 0;
> + cell->nbits = 0;
> + } else {
> + ret = ofnode_read_u32_index(args.node, "bits", 1, &cell->nbits);
> + if (ret)
> + return -EINVAL;
> +
> + if (cell->bit_offset + cell->nbits > cell->size * 8)
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> diff --git a/include/nvmem.h b/include/nvmem.h
> index e6a8a98828b..dd82122f16f 100644
> --- a/include/nvmem.h
> +++ b/include/nvmem.h
> @@ -26,11 +26,15 @@
> * @nvmem: The backing storage device
> * @offset: The offset of the cell from the start of @nvmem
> * @size: The size of the cell, in bytes
> + * @bit_offset: Bit offset within the cell (0 for byte-level access)
> + * @nbits: Number of bits to use (0 for byte-level access)
> */
> struct nvmem_cell {
> struct udevice *nvmem;
> unsigned int offset;
> size_t size;
> + unsigned int bit_offset;
> + unsigned int nbits;
> };
Please also update the documentation for read/write.
> struct udevice;
More information about the U-Boot
mailing list