[U-Boot] Bitfield macros (was: Re: [PATCH v3 02/10] armv7: add miscellaneous utility macros)

[Simon Glass]

On Tue, May 17, 2011 at 9:45 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon Glass,
>
> In message <BANLkTim_uco8YXdPLmtg2uaJ9odU+ZXwwg at mail.gmail.com> you wrote:
>>
>> > Maybe we can agree to use these existing macros then instead of
>> > inventing new ones with the same functionality.
>>
>> The existing macros do not have enough functionality in my view. If we
>> seriously want people to use these then I believe they need to be
>> enhanced to be easier to use and more powerful.
>
> Please read what I wrote. Pay special attention to the "the same
> functionality" part.

I do not propose that we simply create new macros with the same
functionality. That would be a waste of time.

Let me set out my stall here. I think we need a more powerful set of
bitfield macros in U-Boot.

Bitfields done with masks are not the best since:

1. They require the manual creation of masks which is error-prone and pointless
2.. They allow changing of non-contiguous bits which is seldom useful

I believe we should take advantage of the fact that generally
bitfields are contiguous groups of bits within a single word. If you
look at clrsetbits_le32(), I would make these points:

1. Generally native endian is what is wanted, so putting le32() on the
end seems unnecessary
2. It has three arguments: addr, clear, set. The last two are bitmasks
which must be manually created
3. The clear and set arguments are dependent and yet no advantage is
taken of the fact that they are presumably related to the same
bitfield.

Here's a bit of code I found in U-Boot which illustrates this to some extent:

	/* Trigger TIMER_0 by writing A5 to OCPW */
	clrsetbits_be32(&gpt0->emsr, 0xff000000, 0xa5000000);

In fact I would go so far as to say that clrsetbits_le32() is for a
different purpose than adjusting bitfield values. Clearly it is more
powerful than we need, and yet harder to use for our common case.

What could we do instead? I suggest we allow the definition of a
bitfield as a high bit and a low bit, with the bitfield extended
between these two, inclusive, values. So imagine lo and hi for
argument's sake. This is nice and natural because the average SOC
datasheet is full of things like this for example:

Bit       Meaning
8:7       UART type: 0 = blah blah, 1 = something else, ...
6:2       UART word length
1:0       UART stop bits

So a bitfield like 23:16 has hi=23 and lo=16. It is 8 bits long. This
is easily converted into:

- shift (just use lo)
- width (hi - lo + 1)
- 'raw' mask (-1U >> (32 - width))
- 'cooked' mask: raw mask << shift

We can use macros to make this easy, like this:

#define UART_SPEED_HI 23
#define UART_SPEED_LO 16

then allow passing of just UART_SPEED to the macro, from which it gets
the _LO and _HI parts automatically. We can actually go further if we
don't mind a bit of magic and use something like:

#define UART_SPEED_BITS 23:16

from which we can extract these two values automatically. Either
method allows us to pass UART_SPEED to the macros as a complete
definition of the bitfield. We can then use the macros to set values,
use enums and all the normal C things we like with less fear of
complication or error.

I need not dwell on the advantages of replacing all the ad-hoc
bitfield manipulation with something common, tested and robust.

Comments welcome.

Regards,
Simon

>
>> > Do you know of examples of such more complex definitions in the Linux
>> > kernel code?
>>
>> Which complex definitions? I was suggesting adding a more powerful
>> group of bitfield macros to U-Boot. It would be better if they were
>> easier to use rather than more complex.
>
> And I asked if any such "more powerful group of bitfield macros" i
> used in Linux?

Not that I can see. Is that important?

>
>> I think clrbits and setbits are a start but they are primitive and we
>> can do better. If we do better, people will use the better options. If
>> lots of people use them then we can start requiring that people do. In
>> the end we get a consistent way of accessing SOC registers at the bit
>> level, as we now do at the word level.
>
> I understand and accept your opinion, but I do not share it.
>
>
> 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
> Program maintenance is an entropy-increasing process,  and  even  its
> most skilfull execution only delays the subsidence of the system into
> unfixable obsolescence.       - Fred Brooks, "The Mythical Man Month"
>