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