Dear Wolfgang, On Tuesday 07 June 2011 04:09 PM, Wolfgang Denk wrote: > Dear Aneesh V, > > In message<4dede8d9.7030...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
