Dear Wolfgang,
On Tuesday 07 June 2011 09:10 PM, Wolfgang Denk wrote:
Dear Aneesh V,
In message4DEE161B.2050402@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