On Tue, Jun 14, 2011 at 6:53 AM, Wolfgang Denk <w...@denx.de> wrote:
> Dear Aneesh V,
>
> In message <4df7488a.6000...@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: w...@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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
