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