[PATCH v2 2/6] misc: Add support for bit fields in NVMEM cells
Aswin Murugan
aswin.murugan at oss.qualcomm.com
Tue Feb 17 20:05:35 CET 2026
Hi Sean,
Thanks for your feedback.
To address stack smashing, which occurs when the cell size is larger
than 4 bytes, I will revert the `nvmem_cell_read()` call in
`reboot_mode_get()` to pass `sizeof(*mode)` as the size, consistent with
the original implementation.
Inside `nvmem_cell_read()`, will modify the validation check. Instead of
enforcing `size == cell->size`, will check if `cell->size` is within the
passed `size` (i.e., `cell->size <= size`) to correctly support our
1-byte read case where the buffer is 4 bytes.
Also, i2c_eeprom_read() and misc_read() will continue to request a read
for cell->size instead of the passed buffer size, ensuring we only read
the valid data from the hardware.
Regarding the bit field logic, as you suggested will simplify the
bit‑field handling to max u32 size, which significantly reduces the code
complexity. Given this simplification, is it still necessary to keep
this bit‑manipulation logic behind a separate config option?
Kindly confirm if this approach aligns with your expectations.
Thanks,
Aswin
On 2/13/2026 9:04 PM, Sean Anderson wrote:
> 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