This patch series adds bitfield operations to the Tegra code and modifies the existing code to use them. These operations are highly desirable for many reasons which I attempt to explain here.
U-Boot already has some basic macros for dealing with bitfields - setbits(), clrbits() and family. These used a supplied mask for setting and clearing bits.
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. The first parameter is the mask and the second is the value to set it to. It would be better to define the bitfield (as bits 31 to 24) and then have a macro to set the value to 0xa5.
/* 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. 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 = something, 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 a:
- 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 macro magic and use something like:
#define UART_SPEED_RANGE 23:16
from which we can extract these two values automatically. (would _BITS be a better suffix?). 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.
What do compilers make of this? My examination with gcc on ARM at least shows that the compiler likes these macros and produces good code.
Why not make these into inline functions? Some of the macros lend themselves to function calls more than others. The amount of code generated is typically small. The macros allow use to use the bitfield 'descriptor' directly in C code without things like bf_lo(UART_SPEED), bf_hi(UART_SPEED), for example. It doesn't seem like inline functions offer many advantages.
Why not use C bitfields? They don't guarantee any particular read/write behaviour which makes them dangerous for I/O. The C standard doesn't talk much about endianness and ordering, and some claim that the generated code is worse than with bitfield macros. Also because they are evil
http://yarchive.net/comp/linux/bitfields.html
What does U-Boot do at the moment? Well, lots of things.
Example 1: This from OMAP4:
#define DMM_LISA_MAP_SYS_SIZE_MASK (7 << 20) #define DMM_LISA_MAP_SYS_SIZE_SHIFT 20 #define DMM_LISA_MAP_SYS_ADDR_MASK (0xFF << 24)
section = __raw_readl(DMM_LISA_MAP_BASE + i*4); addr = section & DMM_LISA_MAP_SYS_ADDR_MASK; /* See if the address is valid */ if ((addr >= OMAP44XX_DRAM_ADDR_SPACE_START) && (addr < OMAP44XX_DRAM_ADDR_SPACE_END)) { size = ((section & DMM_LISA_MAP_SYS_SIZE_MASK) >> DMM_LISA_MAP_SYS_SIZE_SHIFT);
Example 2: This from netvia.c:
/* some sane bit macros */ #define _BD(_b) (1U << (31-(_b))) #define _BDR(_l, _h) (((((1U << (31-(_l))) - 1) << 1) | 1) & ~((1U << (31-(_h))) - 1))
#define _BW(_b) (1U << (15-(_b))) #define _BWR(_l, _h) (((((1U << (15-(_l))) - 1) << 1) | 1) & ~((1U << (15-(_h))) - 1))
#define _BB(_b) (1U << (7-(_b))) #define _BBR(_l, _h) (((((1U << (7-(_l))) - 1) << 1) | 1) & ~((1U << (7-(_h))) - 1))
#define _B(_b) _BD(_b) #define _BR(_l, _h) _BDR(_l, _h)
#define PD_GP_INMASK 0 #define PD_GP_OUTMASK _BWR(3, 15) #define PD_SP_MASK 0 #define PD_GP_OUTVAL (_BW(3) | _BW(5) | _BW(7) | _BWR(8, 15)) #define PD_SP_DIRVAL 0
ioport->iop_pddat = PD_GP_OUTVAL; ioport->iop_pddir = PD_GP_OUTMASK | PD_SP_DIRVAL; ioport->iop_pdpar = PD_SP_MASK;
Example 3: This from bcm570x:
#define BIT_NONE 0x00 #define BIT_0 0x01 #define BIT_1 0x02 #define BIT_2 0x04 #define BIT_3 0x08 #define BIT_4 0x10 #define BIT_5 0x20 #define BIT_6 0x40 #define BIT_7 0x80 #define BIT_8 0x0100 #define BIT_9 0x0200 #define BIT_10 0x0400 #define BIT_11 0x0800 (and so on)
#define BCM540X_DSP_FILTER_DCOFFSET (BIT_10 | BIT_11) #define BCM540X_DSP_FILTER_FEXT3 (BIT_8 | BIT_9 | BIT_11)
#define BCM540X_AUX_100BASETX_HD (BIT_8 | BIT_9)
switch (Value32 & BCM540X_AUX_SPEED_MASK) { case BCM540X_AUX_100BASETX_HD:
Example 4: lxNpeA.h:
/** set the rxBitField port */ #define IX_NPE_A_RXBITFIELD_PORT_SET( rxbitfield, port ) \ do { \ (rxbitfield) &= ~PORT_MASK; \ (rxbitfield) |= (((port) << 24) & PORT_MASK); \ } while(0)
/** Mask to acess the rxBitField vcId */ #define VCID_MASK 0x00ff0000
/** return the rxBitField vcId */ #define IX_NPE_A_RXBITFIELD_VCID_GET( rxbitfield ) \ (((rxbitfield) & VCID_MASK) >> 16)
(admittedly I struggled to find who uses this code...)
Why make these Tegra-specific? Well, you have to start somewhere. If this patch series survives review then maybe we can look at expanding to other ARM SOCs, 64-bit CPUs and big-endian implementations.
The attached patch series serves as a basic illustration of the definition and use of these macros. These macros could be expanded a little further to add more functionality and convenience, but simple is best. Also included is a basic test program which you can run with the native compiler like this:
$ cd test $ make
Simon Glass (5): Tegra2: Add bitfield access macros Tegra2: Add microsecond timer functions Tegra2: Add more clock support Tegra2: add additional pin multiplexing features Tegra2: Use clock and pinmux functions to simplify code
arch/arm/cpu/armv7/tegra2/Makefile | 2 +- arch/arm/cpu/armv7/tegra2/ap20.c | 92 +++------- arch/arm/cpu/armv7/tegra2/clock.c | 163 +++++++++++++++++ arch/arm/cpu/armv7/tegra2/pinmux.c | 54 ++++++ arch/arm/cpu/armv7/tegra2/timer.c | 27 ++- arch/arm/include/asm/arch-tegra2/bitfield.h | 151 +++++++++++++++ arch/arm/include/asm/arch-tegra2/clk_rst.h | 129 +++++-------- arch/arm/include/asm/arch-tegra2/clock.h | 264 +++++++++++++++++++++++++++ arch/arm/include/asm/arch-tegra2/pinmux.h | 156 +++++++++++++++- arch/arm/include/asm/arch-tegra2/timer.h | 34 ++++ board/nvidia/common/board.c | 65 +++---- test/Makefile | 36 ++++ test/bitfield.c | 230 +++++++++++++++++++++++ 13 files changed, 1204 insertions(+), 199 deletions(-) create mode 100644 arch/arm/cpu/armv7/tegra2/clock.c create mode 100644 arch/arm/cpu/armv7/tegra2/pinmux.c create mode 100644 arch/arm/include/asm/arch-tegra2/bitfield.h create mode 100644 arch/arm/include/asm/arch-tegra2/clock.h create mode 100644 arch/arm/include/asm/arch-tegra2/timer.h create mode 100644 test/Makefile create mode 100644 test/bitfield.c