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

[Simon Glass]

On Tue, Jun 14, 2011 at 6:53 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Aneesh V,
>
> In message <4DF7488A.6000909 at 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
>