[U-Boot] [PATCH v3 02/10] armv7: add miscellaneous utility macros

Simon Glass
2011-06-14T15:11:50Z

On Tue, Jun 14, 2011 at 6:53 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Aneesh V,
>
> In message <4DF7488A.6000909@ti.com> you wrote:
>>
>> Yes. I have seen those macros. But more often than not the bit field is
>> more than 1 bit wide and the value to be set is not necessarily all 0's
>> or all 1's. That's why I have to use clrsetbits_*()
>
> I see. ?In such a case (and only then) clrsetbits_*() is indeed the
> right choice.
>
>> The problem I have to deal with is different. get_bit_field() was
>> intended to extract bit fields from an integer. So, the target usage
>> will be something like this(where a, b, and c are bit fields in
>> register my_reg)
>>
>> u32 my_reg, a_val, b_val, c_val;
>>
>> u32 my_reg = readl(my_reg_addr);
>>
>> a_val = get_bit_field(my_reg, a_mask);
>> b_val = get_bit_field(my_reg, b_mask);
>> c_val = get_bit_field(my_reg, c_mask);
>>
>> Do you see an alternative method for doing this using the standard
>> macros?
>
> Please see the example given here:
>
> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/101146
>
> Looking closer, the "FIELD_VAL" macro alone will probably not suffice,
> as you need both shift directions, like that:
>
> ? ? ? ?#define FIELD_SHIFT ? ? 16
> ? ? ? ?#define FIELD_MASK ? ? ?0xF
>
>
> ? ? ? ?#define FIELD_BITS(x) (x << 16)
> ? ? ? ?#define FIELD_MASK FIELD_BITS(0xF)
> ? ? ? ?#define FIELD_VAL(x) ((x & FIELD_MASK) >> 16)

Hi Wolfgang,

I think you have FIELD_MASK being two meanings: the un-shifted or
'raw' mask, and the shifted mask. So perhaps:

? ? ? ?#define FIELD_SHIFT ? ? 16
? ? ? ?#define FIELD_RAWMASK ? ? ?0xF

? ? ? ?#define FIELD_BITS(x) (x << 16)
? ? ? ?#define FIELD_MASK FIELD_BITS(FIELD_RAWMASK)
? ? ? ?#define FIELD_VAL(x) ((x & FIELD_MASK) >> 16)

or with the 16 factored properly, but even harder to read:

? ? ? ?#define FIELD_SHIFT ? ? 16
? ? ? ?#define FIELD_RAWMASK ? ? ?0xF

? ? ? ?#define FIELD_BITS(x) (x << FIELD_SHIFT)
? ? ? ?#define FIELD_MASK FIELD_BITS(FIELD_RAWMASK)
? ? ? ?#define FIELD_VAL(x) ((x & FIELD_MASK) >> FIELD_SHIFT)

(note that FIELD_BITS should arguably mask after the shift).

When you have a lot of these definitions in a row you have mentally
check the bit width of the mask:

? ? ? ?#define FIELD1_RAWMASK ? ? ?0xF
? ? ? ?#define FIELD1_SHIFT ? ? 16
? ? ? ?#define FIELD2_RAWMASK ? ? ?0x1F
? ? ? ?#define FIELD2_SHIFT ? ? 11
? ? ? ?#define FIELD3_RAWMASK ? ? ?0x1F
? ? ? ?#define FIELD3_SHIFT ? ? 7
? ? ? ?#define FIELD4_RAWMASK ? ? ?0x1F
? ? ? ?#define FIELD4_SHIFT ? ? 1

? ? ? ?#define FIELD1_BITS(x) (x << FIELD1_SHIFT)
? ? ? ?#define FIELD1_MASK FIELD_BITS(FIELD1_RAWMASK)
? ? ? ?#define FIELD1_VAL(x) ((x & FIELD1_MASK) >> FIELD1_SHIFT)
? ? ? ?#define FIELD2_BITS(x) (x << FIELD2_SHIFT)
? ? ? ?#define FIELD2_MASK FIELD_BITS(FIELD2_RAWMASK)
? ? ? ?#define FIELD2_VAL(x) ((x & FIELD2_MASK) >> FIELD2_SHIFT)
...

Is the above correct, or do fields overlap or not cover fully?
(exercise for reader) (see [1] below)

So I think is better to think of the width rather than the mask.

? ? ? ?#define FIELD_SHIFT ? ? 16

#define FIELD_RAWMASK (1U << FIELD_WIDTH) - 1
? ? ? ?#define FIELD_BITS(x) (x << FIELD_WIDTH)
? ? ? ?#define FIELD_MASK FIELD_BITS(FIELD_RAWMASK)
? ? ? ?#define FIELD_VAL(x) ((x & FIELD_MASK) >> FIELD_WIDTH)

because then it is more obvious:

? ? ? ?#define FIELD1_WIDTH ? ? ?4
? ? ? ?#define FIELD1_SHIFT ? ? 16
? ? ? ?#define FIELD2_WIDTH ? ? ?5
? ? ? ?#define FIELD2_SHIFT ? ? 11
? ? ? ?#define FIELD3_WIDTH ? ? ?5
? ? ? ?#define FIELD3_SHIFT ? ? 7
? ? ? ?#define FIELD4_WIDTH ? ? ?5
? ? ? ?#define FIELD4_SHIFT ? ? 1

And now it is a little easier to see that 11+5 = 16, so FIELD2 is ok;
7+5=12 so FIELD3 overlaps, 1+5=6 so FIELD4 isn't big enough. It's
still not as good as just numbering your bits on little-endian archs,
but I think we have had that discussion.

I think BITS and VAL are not very descriptive which is why I suggested
pack and unpack at the time.

But don't get me started talking about bit fields.

Regards,
Simon

[1] This is why macros are so nice:

define once:
#define bf_mask(field) ((field ## _RAWMASK) << field ## _SHIFT)
#define bf_val(field, val) (((val) & bf_mask(field)) >> field ## _SHIFT)
#define bf_bits(field, val) (((val) << field ## _SHIFT) & bf_mask(field))

then:
? ? ? ?#define FIELD1_BITS(x) bf_bits(FIELD1, x)
? ? ? ?#define FIELD1_MASK bf_mask(FIELD1)
? ? ? ?#define FIELD1_VAL(x) bf_val(FIELD1, x)
? ? ? ?#define FIELD2_BITS(x) bf_bits(FIELD2, x)
? ? ? ?#define FIELD2_MASK bf_mask(FIELD2)
? ? ? ?#define FIELD2_VAL(x) bf_val(FIELD2, x)
...

>
> The code would then look something like this:
>
> ? ? ? ?my_reg = readl(my_reg_addr);
>
> ? ? ? ?a_val = A_VAL(my_reg);
> ? ? ? ?b_val = B_VAL(my_reg);
> ? ? ? ?c_val = C_VAL(my_reg);
>
> ...or similar.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> "I dislike companies that have a we-are-the-high-priests-of-hardware-
> so-you'll-like-what-we-give-you attitude. I like commodity markets in
> which iron-and-silicon hawkers know that they exist to ?provide ?fast
> toys for software types like me to play with..." ? ?- Eric S. Raymond
>