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

[Aneesh V]

Hi Wolfgang,

On Tuesday 07 June 2011 12:20 AM, Wolfgang Denk wrote:
> Dear Aneesh V,
>
> In message<4DECF8DA.9030806 at ti.com>  you wrote:
>>
>>>> I really dislike such "useful" helpers, because they make the code
>>>> unreadable. Being nonstandrd, nbody knows what they are doing, so you
>>>> always will have to look up the implementation before you can continue
>>>> reading the code. It's a waste of time an resources.
>>
>> What if I change the above to something like this(please note there
>> isn't any Linux, U-Boot substitute for this):
>>
>> +/* extract a bit field from a bit vector */
>> +#define get_bit_field(nr, field)\
>> +	(((nr)&  (field##_MASK))>>  (field##_OFFSET))
>> +
>> +/* Set a field in a bit vector */
>> +#define set_bit_field(nr, field, val)\
>> +	do { \
>> +		(nr) = ((nr)&  ~(field##_MASK)) | (((val)<<  (field##_OFFSET))\
>> +			&  (field##_MASK));\
>> +	} while (0);
>> +
>>
>> And use it like this:
>> assoc  = get_bit_field(ccsidr, CCSIDR_ASSOCIATIVITY);
>> set_bit_field(ccsidr, CCSIDR_ASSOCIATIVITY, assoc + 1);
>>
>> Isn't it more intuitive and almost self-explanatory now.
>
> To me it is definitely NOT self-explanatory.
>
> Actually I cannot even understand the argument names, now how it's
> supposed to be used.  I would interpret "nr" as "number"; in the
> context of a bit field probably a bit number?  Wrong guess...
>
> It's highly cryptic and will only work with very special #defines
> providing definitions of foo_MASK and foo_OFFSET.
>
> It is not clear that you can use this on plain simple memory stored
> variables only, and that you must not use this on any device
> registers; yet your example looks as if this were intended to be used
> on registers - but then we would need proper I/O accessors.

No. it's not meant to be directly used on registers. Please see below.

>
> And there are still many cases where you cannot use this because for
> example evaluating several bits from the same object will cause
> repeated accesses, which may have side effects (when accessing device
> registers).
>
> And, last but not least: they are nonstandard.  I still don't
> understand why you cannot use the standard macros that already exist
> in Linux and in U-Boot, here for example clrbits*(), setbits*() and
> clrsetbits*() ?

As I had mentioned in a previous mail, please note that the above
macros are not for the same use-case as clrsetbits*() or friends (I had
one macro that did something similar to clrsetbits*() and I intent to
remove that in the next revision)

The above macros are for bit-field manipulation in a C integer variable
- nothing more.

However, the typical use of this is for extracting bit-fields from
an IO register that is already read into an integer variable (instead
of reading the IO register multiple times). And similarly for write.

So, if I have to write 5 different fields in a register I first write
them into a variable and finally call writel() instead of making 5
clrsetbits*() calls.

There aren't any standard routines available for this need in
Linux or U-Boot. I think you had agreed on this fact sometime back.

>
>> If you still don't like these as standard generic macros, how about
>> having these macros just for OMAP with these names and use them only
>> for OMAP code?
>
> Would that make it anything better?  If it's not good enough for
> general use, we can still use it for OMAP?  Like: oh, it's ony OMAP,
> so code quality does not matter?  I think that's not good reasoning.

No. It was not about code quality. The question was whether these
macros were generic enough to be used as the standard U-boot ones. The
key question is how do you represent bit fields. There are different
alternatives for this.

a. bit range (say 5:3)
b. shift(3) and field width(3)
c. shift(3) and mask(0x38)

We traditionally use (c) and we have auto-generated defines in this form.
So, my macros use this format. I was not sure if other SoCs follow the
same approach. That's why I suggested making them OMAP specific if you
think (c) is not the standard approach.

best regards,
Aneesh