[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Anton Staaf]

On Tue, Jul 12, 2011 at 2:18 PM, Wolfgang Denk <wd at denx.de> wrote:
>
> Dear Anton Staaf,
>
> In message <CAF6FioVcfgxzep7dhgxYR+cjaf0nQ8LYBxKvEaqycz8NOoSWDA at mail.gmail.com> you wrote:
> >
> > That makes sense to me.  Would an alternative that uses the "width" and
> > "size" of the field be acceptable?  Then there is a well understood (on both
> > types of architectures) mapping from these values to the mask and shift
> > required to access portions of a register and/or variable in memory.
>
> "width" and "size" seem indentical to me here.  Do you mean "width"
> and "offset" or so?  The problem still remains.  People who are used
> to number bits from left to right will also tend to count bit offsets
> in the same direction.

Yes, I meant shift, not size.  While it may be the case that some
people count bits from the left, I don't think I've ever seen code
that tries to right shift a value into place by that offset,
everything seems to use the (value << shift) idiom.  In which case
specifying a shift (offset) value seems pretty safe.

> In the end, the most simple and truly portable way is specifying the
> masks directly.  What's wrong with definitions like
>
>        #define SCFR1_IPS_DIV_MASK      0x03800000
>        #define SCFR1_PCI_DIV_MASK      0x00700000
>        #define SCFR1_LPC_DIV_MASK      0x00003800
>
> etc.?
>
> I can actually read these much faster that any of these bit field
> definitions.

The only problem with this is that there is one piece of missing
information, which is how do you get the value of the field that is
masked as if it were a 4-bit or 3-bit number.  That is, if I want the
IPS_DIV value, I probably want:

   (value & SCFR1_IPS_DIV_MASK) >> 20

Likewise, if I want to set the IPS divisor to 5 say, I would have to do:

   (value & ~SCFR1_IPS_DIV_MASK) | (5 << 20)

In both cases the value 20 needs to come from somewhere.  So you would
probably end up having:

   #define SCFR1_IPS_DIV_MASK      0x03800000
   #define SCFR1_IPS_DIV_SHIFT      20

and

   (value & SCFR1_IPS_DIV_MASK) >> SCFR1_IPS_DIV_SHIFT
   (value & ~SCFR1_IPS_DIV_MASK) | (5 << SCFR1_IPS_DIV_SHIFT)

And I think it would be great if these could turn into:

   field_value = GET_FIELD(register_value, SCFR1_IPS_DIV)
   register_value = SET_FIELD(register_value, SCFR1_IPS_DIV, 5)

The GET and SET macros would in my opinion be more readable than a lot
of shifting and oring.  And could be used with existing U-Boot
register access functions:

    writel(SET_FIELD(readl(&reg), SCFR1_IPS_DIV, 5), &reg)

Or, and I think this is something you have nacked in that past, but I
like it and hope you don't mind me ending with it, this could
eventually be:

    SET_REGISTER_FIELD(&reg, SCFR1_IPS_DIV, 5)

> > --00504502e3b9570ace04a7e593ca
> > Content-Type: text/html; charset=ISO-8859-1
> > Content-Transfer-Encoding: quoted-printable
>
> Please stop posting HTML.

Ack, sorry about that, it's my least favorite feature of web mail.  :(

>
> 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
> News is what a chap who doesn't care much  about  anything  wants  to
> read. And it's only news until he's read it. After that it's dead.
>                           - Evelyn Waugh _Scoop_ (1938) bk. 1, ch. 5