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

[Aneesh V]

Dear Wolfgang,

On Tuesday 07 June 2011 09:10 PM, Wolfgang Denk wrote:
> Dear Aneesh V,
>
> In message<4DEE161B.2050402 at ti.com>  you wrote:
>>
>>>> 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.
>
> You can use in_le32() to read the register in a variable, a number of
> macros operating on that variable, and then a out_le32() to write it
> back.
>
> It's logically 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*()
>
> See above.
>
> If you ever want to use your macros on device registers, you MUST use
> proper memory barriers.  This will probably ruin your idea to combine
> the access into a single write.  So you can just work on a variable as
> suggested before.
>
>> 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 4 - with standard macros, and proper defines:
>
> 	unsigned int r = in_le32(reg);		/* or readl() if you like */
>
> 	clrsetbits_le32(&r, a_mask, a_val);
> 	clrsetbits_le32(&r, b_mask, b_val);
> 	clrsetbits_le32(&r, c_mask, c_val);
>
> 	out_le32(reg, r);

I still don't think this is the 'right' solution for my problem. I don't
like the fact that clrsetbits_le32() introduces a lot of un-necessary
'volatile's.

Yes, it's about the 'efficiency'. May be it doesn't count in some
cases. But, may be it counts in some other cases. Basically, I don't
like to sacrifice 'efficiency' unless the cost for achieving it is very
high. I don't think having an extra helper function is a big cost.
Neither do I believe that readability suffers in this case.

If you still insist, I can use clrsetbits_le32() in the interest
of getting this to a closure.

>
> Actually solution 3 looks best to me.
>
>> 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.
>
> In case of side effects you can use solution 4.
>
> We should not bother here about wether this is "efficient" or "not
> efficient".  Probably none opf this code is ever time critical, not to
> the extent of a few additional instructions.
>
>> 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.
>
> I agree that 1 is ugly and error prone, but there is no need to use
> it.
>
> I repeat:  we have a set of powerful macros ready available.  Just use
> them.

We have a set of powerful macros designed for bit-field accesses in IO
egisters.

But, what I am looking for is a set of macros for bit-field operations
on C integer variables without the un-necessary overhead of IO register
accesses. I am looking for missing APIs in bitops.h not anything from
io.h

best regards,
Aneesh