[U-Boot] [PATCH 1/5] Tegra2: Add bitfield access macros

[Simon Glass]

Hi Wolfgang,

On Tue, Jun 7, 2011 at 1:00 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon Glass,
>
> In message <BANLkTimN4NVD43B2Le7_ci9Picjd6pmuXA at mail.gmail.com> you wrote:
>>
>> >        clrsetbits_le32(&my_device->ctrl, FIELD_MASK, FIELD_VAL(6));
>>
>> We now have a computed mask which is good, thank you.
>>
>> But the FIELD is specified twice in the same statement. Can we
>> therefore please take this a step further and write something like
>> this?
>>
>> clrsetfield_le32(&my_device->ctrl, FIELD, 6);
>
> I think I should provide a little moe explanation why I dislike your
> suggestion.  With "FOO_MASK, FOO_VAL()" it is obvious to the
> reader that we are dealing ith some (bit) mask and some value, and it
> is also trivial to locate the respective definitions of these macros,
> because you know their exact names.
>
> With "FOO, 6" you know nothing.  Searching for "FOO" in the source
> code and header files will probably return a ton of unrelated
> references, and you will have to read (and understand) the definition
> of the bitfield macro and follow it's preprocessor trickery with
> combining some magic name parts until you finally know what to look
> for.

I don't agree. Going back to the original patch, the macro allow us to
write this code to read/write bias time for example (showing header
and C file):

#define UTMIP_BIAS_PDTRK_COUNT_RANGE		7:3

	bf_writel(UTMIP_BIAS_PDTRK_COUNT, params->bias_time,
			&usbctlr->utmip_bias_cfg1);

and we also have a lot of things like this (Note: this code line is
made up and isn't actually in the code):
        return bf_readl(UTMIP_BIAS_PDTRK_COUNT, &usbctlr->utmip_bias_cfg1);

By the way a grep for UTMIP_BIAS_PDTRK_COUNT pulls up exactly what I
would expect:

$ ack UTMIP_BIAS_PDTRK_COUNT |more
arch/arm/include/asm/arch-tegra2/usb.h:162:#define
UTMIP_BIAS_PDTRK_COUNT_RANGE		7:3
board/nvidia/common/usb.c:202:	bf_writel(UTMIP_BIAS_PDTRK_COUNT,
params->bias_time,
$

What you are asking for is I think something like:

#define UTMIP_BIAS_PDTRK_COUNT_SHIFT 4
#define UTMIP_BIAS_PDTRK_COUNT_MASK (0xf << UTMIP_BIAS_PDTRK_COUNT_SHIFT)
#define UTMIP_BIAS_PDTRK_COUNT_VAL(v) \
      ((v << UTMIP_BIAS_PDTRK_COUNT_SHIFT) & UTMIP_BIAS_PDTRK_COUNT_MASK)
#define UTMIP_BIAS_PDTRK_COUNT_EXTRACT(v) \
      ((v & UTMIP_BIAS_PDTRK_COUNT_MASK) >> UTMIP_BIAS_PDTRK_COUNT_SHIFT)

clrsetbits_le32(&usbctlr->utmip_bias_cfg1, UTMIP_BIAS_PDTRK_COUNT_MASK,
        UTMIP_BIAS_PDTRK_COUNT_VAL(params->bias_time));
return UTMIP_BIAS_PDTRK_COUNT_EXTRACT(readl(&usbctlr->utmip_bias_cfg1))


Going further back now to use explicit shifts in the C code, we have:

#define UTMIP_BIAS_PDTRK_COUNT_SHIFT 4
#define UTMIP_BIAS_PDTRK_COUNT_MASK (0xf << UTMIP_BIAS_PDTRK_COUNT_SHIFT)

clrsetbits_le32(&usbctlr->utmip_bias_cfg1, UTMIP_BIAS_PDTRK_COUNT_MASK,
        params->bias_time << UTMIP_BIAS_PDTRK_COUNT_SHIFT);
return (readl(&usbctlr->utmip_bias_cfg1) & UTMIP_BIAS_PDTRK_COUNT_MASK) >>
        UTMIP_BIAS_PDTRK_COUNT_SHIFT;

and the original idea again:

#define UTMIP_BIAS_PDTRK_COUNT_RANGE		7:3

	bf_writel(UTMIP_BIAS_PDTRK_COUNT, params->bias_time,
			&usbctlr->utmip_bias_cfg1);
        return bf_readl(UTMIP_BIAS_PDTRK_COUNT, &usbctlr->utmip_bias_cfg1);

>
> You consider this macro helpful, I consider it obscure, and that's why
> I don't want to have it.

So yes you need to learn the macros, but once you have done that
(which takes maybe 10 minutes if you haven't had your coffee yet)
things are easier. Also, the datasheet has things like:

Bit:
13:8    UTMIP_BIAS_TCTRL
7:3      UTMIP_BIAS_PDTRK_COUNT
2         UTMIP_VBUS_WAKEUP_TIME

So it is very natural to be able to define fields like this.

Regards,
Simon

>
> 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
> If you want strict real-time behavior, run in the real  time  schedu-
> ling class.  But there are no seatbelts or airbags;  main(){for(;;);}
> can hard hang your system.                          -- Bart Smaalders
>