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

[Aneesh V]

Dear Wolfgang,

On Tuesday 07 June 2011 04:09 PM, Wolfgang Denk wrote:
> Dear Aneesh V,
>
> In message<4DEDE8D9.7030306 at ti.com>  you wrote:
>>
>> 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.
>
> Why cannot we use the existing macros?
>
>> 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.
>
> It does not make much difference to me if you call one macro or
> another 5 times.
>

No it makes a difference. It's 5 writes to a variable typically in an
ARM register + 1 IO access vs 5 IO accesses. It's logically not
equivalent.

Further if the 5 values are constants a smart compiler will fold the
five writes into one write to the ARM register + 1 IO access, which
won't happen if you used 5 clrsetbits*()

Let me give you a solid example:

Problem: We want to read-modify-write an IO register 'reg' affecting 3 
different fields: a, b, and c. The values to be written to the fields
are a_val, b_val, and c_val respectively:

Solution 1 - without any macros:

unsigned int r = readl(reg);
r = (r & ~a_mask) | ((a_val << a_shift) & a_mask)
r = (r & ~b_mask) | ((b_val << b_shift) & b_mask)
r = (r & ~c_mask) | ((c_val << c_shift) & c_mask)
writel(r, reg);

Solution2 - with my macros:

unsigned int r = readl(reg);
set_bit_field(r, a, a_val);
set_bit_field(r, b, b_val);
set_bit_field(r, c, c_val);
writel(r, reg);

Solution3 - with clrsetbits*():

clrsetbits_le32(reg, a_mask, a_val << a_shift);
clrsetbits_le32(reg, b_mask, b_val << b_shift);
clrsetbits_le32(reg, c_mask, c_val << c_shift);


Solution 3 is not acceptable to me because it's clearly not equivalent
to what I want to do. Writing the register 3 times instead of once may
have undesirable side-effects. Even if it worked, it's clearly not
efficient.

If you are forcing me to use solution 1, IMHO you are essentially
forcing me not to use a sub-routine for a task that is repeated many
times in my code, leaving my code to be more error prone and less
readable.

You accuse set_bit_field() of being cryptic. I would say the
implementation of clrsetbits_le32() is even more cryptic with so many
levels of indirection. I think that goes with any sub-routine/API.
You need to read the code/documentation once to know what it does.
After that you take it's functionality for granted and things become
easier for you. If better documentation can improve readability I am
happy to do that.

Also, If you don't like it as a generic API I am willing to make it a
static inline function in my code. But I need a utility function for
this need. If you think the implementation/documentation can be
improved I am willing to work on that too. But please suggest a
solution for this problem.

> It does mater to me to have several incompatible implementations doing
> essentially the same thing.

They are not doing the same thing as explained above.

>
>> 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.
>
> I agree in so far as I am not aware of any such macros in Linux
> either.  But my conclusion is a different one - it boils down to:
> Linux is way more complex than U-Boot, so if they don;t need this, we
> don't need it either.

I am surprised why Linux doesn't have a solution for this. Perhaps the
reason must be the confusion about the representation of a field that
we discussed below. I suspect there may be non-standard local
implementations in different modules.

Also, as somebody already mentioned, can't we do better than Linux?

>
>
>> 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)
>
> d) Value and mask
>
>> 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.
>
> Actually it does not matter.  See my previous message to Simon: you
> can cover all this with the existing macros, and without adding any
> significant overhead.
>
> So far, I did not see a single good argument why any new, nonstandard
> macros would be needed.

Please consider the above example and let me know if I missed any
solution using the existing standard macros.

best regards,
Aneesh