C-macro: set a register field defined by a bit-mask to a given value

Question

I've got 32-bit registers with field defined as bit-masks, e.g.

#define BM_TEST_FIELD 0x000F0000

I need a macro that allows me to set a field (defined by its bit-mask) of a register (defined by its address) to a given value. Here's what I came up with:

#include <stdio.h>
#include <assert.h>

typedef unsigned int u32;

/* 
 * Set a given field defined by a bit-mask MASK of a 32-bit register at address
 * ADDR to a value VALUE.
 */
#define SET_REGISTER_FIELD(ADDR, MASK, VALUE)                                      \
{                                                                                  \
  u32 mask=(MASK); u32 value=(VALUE);                                              \
  u32 mem_reg = *(volatile u32*)(ADDR); /* Get current register value           */ \
  assert((MASK) != 0);                  /* Null masks are not supported         */ \
  while(0 == (mask & 0x01))             /* Shift the value to the left until    */ \
  {                                     /* it aligns with the bit field         */ \
    mask = mask >> 1; value = value << 1;                                          \
  }                                                                                \
  mem_reg &= ~(MASK);                   /* Clear previous register field value  */ \
  mem_reg |= value;                     /* Update register field with new value */ \
  *(volatile u32*)(ADDR) = mem_reg;     /* Update actual register               */ \
}

/* Test case */
#define BM_TEST_FIELD 0x000F0000
int main()
{
  u32 reg = 0x12345678;
  printf("Register before: 0x%.8X\n", reg);/* should be 0x12345678 */
  SET_REGISTER_FIELD(&reg, BM_TEST_FIELD, 0xA);
  printf("Register after: 0x%.8X\n", reg); /* should be 0x123A5678 */
  return 0;
}

Is there a simpler way to do it?

EDIT: in particular, I'm looking for a way to do reduce the run-time computing requirements. Is there a way to have the pre-processor compute the number of required left-shifts for the value?

Answer 1

Why not just put both the mask and the value in the right place?

#define BM_TEST_FIELD (0xfUL << 16)
#define BM_TEST_VALUE (0xaUL << 16)
#define mmioMaskInsert(reg, mask, value) \
   (*(volatile u32 *)(reg) = (*(volatile u32 *)(reg) & ~(mask)) | value)

Then you can just use it like:

mmioMaskInsert(reg, BM_TEST_FIELD, BM_TEST_VALUE);

For sure what you have there is very dangerous. Register writing can often have side-effects, and these operations:

mem_reg &= ~(MASK);
mem_reg |= value;

are actually writing to the register twice, instead of once, like you probably intend to. Also, why isn't a mask of 0 supported? What if I want to write to the whole register (timer count match or something)? Do you have a different macro for that operation? If so, why not use it as part of this system?

Another note - it might be a good idea to apply the mask to the value before sticking it in the register, in case someone passes a value that has more bits than the mask does. Something like:

#define maskInsert(r, m, v) \
  (*(volatile u32 *)(r) = (*(volatile u32 *)r & ~(m)) | ((v) & ~(m)))

Answer 2

I would consider using bitfields to "format" bits for hardware, e.g.:

#include <stdio.h>
#include <inttypes.h>

struct myregister {
    unsigned upper_bits:12;
    unsigned myfield:4;
    unsigned lower_bits:16;
};

typedef union {
    struct myregister fields;
    uint32_t value;
} myregister_t;

int main (void) {
    myregister_t r;
    r.value = 0x12345678;
    (void) printf("Register before: 0x%.8" PRIX32 "\n", r.value);
    r.fields.myfield = 0xA;
    (void) printf("Register after: 0x%.8" PRIX32 "\n", r.value);
    return 0;
}

Edit: Note the follow-up discussion in the comments. There are valid arguments against using bitfields, but, in my opinion, also benefits (especially in syntax, which I value greatly). One should decide based on the circumstances the code will be used in.

Answer 3

If you insist on this specific interface (the position of the field is defined by the mask), then probably the only thing that can be changed/improved in your implementation is the cycle where you shift the value to the proper position (to align it with the mask). Basically, what you have to do is to find the offset expressed in the number of bits, and the shift the value left that number of bits. You used a plain cycle to perform that operation, and instead of explicitly calculating the offset in bits you simply shift the value left 1 bit at each iteration. This will work. However, it might be seen as inefficient, especially for fields that reside in the upper portion of the register, since they will require more iterations of the shifting cycle.

In order to improve the efficiency you can also use any of the rather well-known, potentially more efficient methods to calculate the offset value, as the ones described on this page. I don't know whether this is worth the effort in your case though. It might make your code more efficient, but it might also make it less readable. Decide for yourself.

Answer 4

EDIT: in particular, I'm looking for a way to do reduce the run-time computing requirements. Is there a way to have the pre-processor compute the number of required left-shifts for the value?

Yes:

value *= ((MASK) & ~((MASK) << 1))

This multiplies value by the lowest set bit in MASK. The multiplier is known to be a constant power of 2 at compile time, so this will be compiled as a simple left shift by any remotely sane compiler.