[U-Boot] [PATCH v3 02/10] armv7: add miscellaneous utility macros

[Simon Glass]

On Sun, May 15, 2011 at 11:44 AM, Wolfgang Denk <wd at denx.de> wrote:

> Dear Aneesh V,
>
> In message <1305202276-27784-3-git-send-email-aneesh at 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 at denx.de
> No journaling file system can recover your data if the disk dies.
>                                 - Steve Rago in <D4Cw1p.L9E at plc.com>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>