On Sun, May 15, 2011 at 11:44 AM, Wolfgang Denk wd@denx.de wrote:
Dear Aneesh V,
In message 1305202276-27784-3-git-send-email-aneesh@ti.com you wrote:
add utility macros for:
- bit field operations
- log2n functions
...
+/* extract a bit field from a bit vector */ +#define get_bit_field(nr, start, mask)\
(((nr) & (mask)) >> (start))+/* Set a field in a bit vector */ +#define set_bit_field(nr, start, mask, val)\
do { \(nr) = ((nr) & ~(mask)) | (((val) << (start)) & (mask));\} while (0);I really dislike such "useful" helpers, because they make the code unreadable. Being nonstandrd, nbody knows what they are doing, so you always will have to look up the implementation before you can continue reading the code. It's a waste of time an resources.
Hi Wolfgang,
Please consider my thoughts below on this subject.
It would be good to enhance these helpers to address the bitfield disaster which is a modern SOC.
I agree that having standard helpers is useful and in fact I don't believe the existing clrbits and setbits go far enough. I have noticed that various architectures have their own macros for handling bitfields. Since these are all different (thus you need to read each one to understand the code, as you say) and pan-U-Boot solutions appear to be rejected, we are stuck with bloated, error-prone code full of shifts and masks.
I believe that this problem is getting worse - e.g. USB on Tegra2 writes various fields of about 20 registers to get things up and running. I find translating SOC datasheet register definitions into C code with shifts and masks to be slow and error-prone work. Also we do need to maintain this code, and it gets reused for new SOC variants, etc. So it is not as if it is written once and then buried and forgotten. There is also a tendency to use 'magic' constants rather than #define values or something with a sensible name, then hopefully add a half-hearted comment. This requires constant return looks at the datasheet to see what bits were chosen.
Being a boot loader, charged with basic hardware initialisation, I believe bitfield access primitives should be well-supported by U-Boot.
Would you consider an RFC patch to add pan-U-Boot bitfield operations? Failing that, how about just for ARM?
Regards, Simon
+/*
- Utility macro for read-modify-write of a hardware register
- addr - address of the register
- shift - starting bit position of the field to be modified
- msk - mask for the field
- val - value to be shifted masked and written to the field
- */
+#define modify_reg_32(addr, shift, msk, val) \
do {\writel(((readl(addr) & ~(msk))|(((val) << (shift)) &(msk))),\
(addr));\} while (0);NAK again, for the same reasons.
Note that there are some semi-standardized I/O accessro macros available, at least for some architectures, like clrbits.*(), setbits_(), or clrsetbits().
See for example "arch/arm/include/asm/io.h", "arch/powerpc/include/asm/io.h" for reference.
Instead of reinventing the wheel (just differently shaped) we should rather try and use a single, standardized set of such helpers.
So please use the existing macros instead of inventing new, non-standard ones.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de No journaling file system can recover your data if the disk dies. - Steve Rago in D4Cw1p.L9E@plc.com _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot