Add bitfields, clock and pinmux functions to simplify code

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@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

1.7.3.1

To use these, set things up like this:

struct uart_ctlr *uart = (struct uart_ctlr *)UART_PA_START;

 #define UART_PA_START 0x67000000       /* Physical address of UART */
 #define UART_FBCON_RANGE  5:3          /* Bit range for the FBCON field */
enum {                                  /* An enum with allowed values */

UART_FBCON_OFF,
UART_FBCON_ON,
UART_FBCON_MULTI,
UART_FBCON_SLAVE,
};

This defines a bit field of 3 bits starting at bit 5 and extending down to bit 3, i.e. 5:3

Then:
bf_unpack(UART_FBCON)
- return the value of bits 5:3 (shifted down to bits 2:0)

bf_pack(UART_FBCON, 4)
- return a word with that field set to 4 (so in this case (4 << 3))

bf_update(UART_FBCON, word, val)
- update a field within word so that its value is val.

bf_writel(UART_FBCON, 6, &uart->fbcon)
- set the UART's FBCON field to 6

bf_enum_writel(UART_FBCON, MULTI, &uart->fbcon)
- set the UART's FBCON field to MULTI

---
 arch/arm/include/asm/arch-tegra2/bitfield.h |  151 ++++++++++++++++++
 test/Makefile                               |   36 ++++
 test/bitfield.c                             |  230 +++++++++++++++++++++++++++
 3 files changed, 417 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-tegra2/bitfield.h
 create mode 100644 test/Makefile
 create mode 100644 test/bitfield.c

new file mode 100644
index 0000000..6a1bbfa
@@ -0,0 +1,151 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#ifndef __TEGRA2_BITFIELD_H
+#define __TEGRA2_BITFIELD_H
+
+/*
+ * Macros for working with bit fields. To use these, set things up like this:
+ *
+ * #define UART_PA_START 0x67000000	Physical address of UART
+ * #define UART_FBCON_RANGE  5:3	Bit range for the FBCON field
+ * enum {				An enum with allowed values
+ *	UART_FBCON_OFF,
+ *	UART_FBCON_ON,
+ *	UART_FBCON_MULTI,
+ *	UART_FBCON_SLAVE,
+ * };
+ * struct uart_ctlr *uart = (struct uart_ctlr *)UART_PA_START;
+ *
+ * This defines a bit field of 3 bits starting at bit 5 and extending down
+ * to bit 3, i.e. 5:3
+ *
+ * Then:
+ *    bf_unpack(UART_FBCON)
+ *	- return the value of bits 5:3 (shifted down to bits 2:0)
+ *
+ *    bf_pack(UART_FBCON, 4)
+ *	- return a word with that field set to 4 (so in this case (4 << 3))
+ *
+ *    bf_update(UART_FBCON, word, val)
+ *	- update a field within word so that its value is val.
+ *
+ *    bf_enum_writel(UART_FBCON, MULTI, &uart->fbcon)
+ *	- set the UART's FBCON field to MULTI
+ *
+ *
+ * Why have bitfield macros?
+ *   1. Reability
+ *   2. Maintainability
+ *   3. Less error prone
+ *
+ * For example, this:
+ *
+ *	int RegVal = 0;
+ *	RegVal= readl(UsbBase+USB_SUSP_CTRL);
+ *	RegVal |= Bit11;
+ *	writel(RegVal, UsbBase+USB_SUSP_CTRL);
+ *	if(UsbBase == NV_ADDRESS_MAP_USB3_BASE)
+ *	{
+ *		RegVal = readl(UsbBase+USB_SUSP_CTRL);
+ *		RegVal |= Bit12;
+ *		writel(RegVal, UsbBase+USB_SUSP_CTRL);
+ *	}
+ *
+ * becomes this:
+ *
+ *	bitfield_writel(UTMIP_RESET, 1, &usbctlr->susp_ctrl);
+ *	if (id == PERIPH_ID_USB3)
+ *		bitfield_writel(UTMIP_PHY_ENB, 1, &usbctlr->susp_ctrl);
+ */
+
+/* Returns the low bit of the bitfield */
+#define BITFIELD_LOWBIT(range)	((0 ? range) & 0x1f)
+
+/* Returns the high bit of the bitfield */
+#define BITFIELD_HIGHBIT(range)	(1 ? range)
+
+/* Returns the width of the bitfield (in bits) */
+#define BITFIELD_WIDTH(range)	\
+	(BITFIELD_HIGHBIT(range) - BITFIELD_LOWBIT(range) + 1)
+
+
+/*
+ * Returns the number of bits the bitfield needs to be shifted left to pack it.
+ * This is just the same as the low bit.
+ */
+#define bf_shift(field)	BITFIELD_LOWBIT(field ## _RANGE)
+
+/* Returns the unshifted mask for the field (i.e. LSB of mask is bit 0) */
+#define bf_rawmask(field) (0xfffffffful >> \
+		(32 - BITFIELD_WIDTH(field ## _RANGE)))
+
+/* Returns the mask for a field. Clear these bits to zero the field */
+#define bf_mask(field) \
+		(bf_rawmask(field) << (bf_shift(field)))
+
+/* Unpacks and returns a value extracted from a field */
+#define bf_unpack(field, word) \
+	(((unsigned)(word) >> bf_shift(field)) & bf_rawmask(field))
+
+/*
+ * Packs a value into a field - this masks the value to ensure it does not
+ * overflow into another field.
+ */
+#define bf_pack(field, value) \
+	((((unsigned)(value)) & bf_rawmask(field)) \
+		<< bf_shift(field))
+
+/* Sets the value of a field in a word to the given value */
+#define bf_update(field, word, value)		\
+	((word) = ((word) & ~bf_mask(field)) |	\
+		bf_pack(field, value))
+
+/*
+ * Sets the value of a field in a register to the given value using
+ * readl/writel
+ */
+#define bf_writel(field, value, reg) ({		\
+	u32 *__reg = (u32 *)(reg);			\
+	u32 __oldval = readl(__reg);			\
+	bf_update(field, __oldval, value);		\
+	writel(__oldval, __reg);				\
+	})
+
+/* Unpacks a field from a register using readl */
+#define bf_readl(field, reg)			\
+	bf_unpack(field, readl(reg))
+
+/*
+ * Clears a field in a register using writel - like
+ * bf_writel(field, 0, reg)
+ */
+#define bf_clearl(field, reg) bf_writel(field, 0, reg)
+
+/*
+ * Sets the value of a field in a register to the given enum.
+ *
+ * The enum should have the field as a prefix.
+ */
+#define bf_enum_writel(field, _enum, reg) \
+		bf_writel(field, field ## _ ## _enum, reg)
+
+#endif

new file mode 100644
index 0000000..6d8c6a7
@@ -0,0 +1,36 @@
+# Copyright (c) 2011 The Chromium OS Authors.
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+
+TESTS=bitfield
+
+INC=../arch/arm/include/asm/arch-tegra2
+CFLAGS=-DDEBUG -I$(INC)
+
+all: tests run
+
+tests: $(TESTS)
+
+bitfield: bitfield.o
+
+bitfield.o: $(INC)/bitfield.h
+
+run:
+	@echo "Running tests $(TESTS)"
+	@./bitfield
+	@echo "Tests completed."

1.7.3.1

Dear Simon Glass,

Sorry, but this is a highly unportable, and completely unreadable.

OK. Assuming reading from UART_FBCON would return the value 0x16000038 or 0b0001.0110.0000.0000.0000.0000.0011.1000. What is the correct and expected return value from your macro?

You say 5:3 means "3 bits starting at bit 5 and extending down to bit 3".

With your ARM view of things you might expect the outcome to be 0x7 = 0b111 because for you bit 0 is the LSB, and your counting "from left to right", and "down" for you means "in direction of less significant bits". But please note, that for the Power Architecture, bit 0 denotes the MSB, so the _expected_ (from the documentation) result of the macro would be 0x5 = 0b101, and your notion of "down to bit 3" is completely unexpected as we are movin in the direction or more significan bits, where "down" is simply wrong in my understanding.

Your design probably does not consider that different architectures have different understandings of how to number bits.

In addition, in portable C code there is no standardizartion for bit
fields: nothing about alignment, layout, padding, maximum bit numbers, or anything. The whole implementationm is compiler-specific.

It would be nice if this worked, alas it doesn't. At least not as you expect it.

Well, such code would be rejected right from the beginning. We don;t allow the use of "base address plus offset" to access structured data like mapped device registers, instead we require the use of proper I/O accessors.

And in which way is this more readable, better maintainable, and/or less error prone then when using the proper standard macros which are ready available in U-Boot:

	setbits_le32(&usbctlr->susp_ctrl, UTMIP_RESET);
	if (id == PERIPH_ID_USB3)
		setbits_le32(&usbctlr->susp_ctrl, UTMIP_PHY_ENB)

Yes, you may need slightly different definitions of UTMIP_RESET and UTMIP_PHY_ENB, but so what?

Sorry, but I am not going to accept this.

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
It is impractical for  the  standard  to  attempt  to  constrain  the
behavior  of code that does not obey the constraints of the standard.
                                                          - Doug Gwyn

[big snip]

Hi Wolfgang,

OK I understand, thank you. How about this approach: we could add a new macro to io.h, something like:

#define clrsetfield_le32(bitfield, addr, value) ...

Then caller can define these in a header file:

#define FIELD_MASK 0xf0000
#define FIELD_SHIFT 16

And use this macro to set the bitfield to 6, for example:

clrsetfield_le32(FIELD, &my_device->ctrl, 6)

(this will simply shift the value left 16 bits and apply the supplied mask)

This captures the essence of bitfields, in that we are abstracting the field under a single name. The change would just add this macro to the io.h header file.

Does this sound better?

Regards,
Simon

Dear Simon Glass,

Sorry, I fail to understand how you envision to use this, and how it would be different from clrsetbits*() ?

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 one wants war.
	-- Kirk, "Errand of Mercy", stardate 3201.7

Hi Wolfgang,

For example this allows us to replace:

clrsetbits_le(&my_device->ctrl, 0xf << 16, 6 << 16)

with:

clrsetfield_le32(FIELD, &my_device->ctrl, 6)

So the two identical shifts are avoided, and the forming of the mask is done once in the define.

Please do try to help me out with a form of bitfield definition that you would be comfortable with. This approach here makes use of clrsetbits*() and extends it in a simple way. If there is another better or more acceptable approach then please do let me know.

Regards,
Simon

Dear Simon Glass,

If you really insist:

	#define FIELD_VAL(x)	(x << 16)
	#define FIELD_MASK	FIELD_VAL(0xF)

	clrsetbits_le32(&my_device->ctrl, FIELD_MASK, FIELD_VAL(6));

In practical use cases, you will probablu not use magg numbers like '6' anyway, and define symbolic names for this anyway, so this would be:

	clrsetbits_le32(&my_device->ctrl, FIELD_MASK, FIELD_VAL_FOO);

or similar.

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

this is basically what we do in the blackfin port. we keep most of the logic in the defines so that we can use the simpler i/o logic without too much hassle.

the only additional thing we do is add a define for the shift in case we need to do something the macros cant provide.
#define FIELD_SHIFT 16
#define FIELD_VAL(x) ((x) << FIELD_SHIFT)
#define FIELD_MASK FIELD_FAL(0xF)
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110607/464a9d1d/attachment.pgp

Hi Wolfgang,

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);

so:

#define clrsetfield_le32(addr, field, data) clrsetbits_le32(add, field ##_MASK, field ## _VAL(data))

How can we combine these two to remove the redundancy?

In our code there is quite a bit of both but yes setting single bit fields to 0 for off, and 1 for on, is common. Also we sometime have things like:

enum {
        OSC_FREQ_12,
        OSC_FREQ_16,
        OSC_FREQ_19_2,
        OSC_FREQ_24,
};

for 2-bit fields (and so on for 3, 4 bit fields).

Regards,
Simon

Dear Simon Glass,

I consider this too specific on one side, and the additional effort to write two arguments too less of an issue to introduce another macro.

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
: ... and it's got weird formatting - Notepad, Write, Works  3  can't
: decipher it, and it's too big to go in DOS Edit. Help!
Install an operating system. :-)                  -- Tom Christiansen

Dear Simon Glass,

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.

You consider this macro helpful, I consider it obscure, and that's why I don't want to have 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
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

Hi,

For what its worth, I also consider the magic under the hood to be too much. It may look clever, but it actually makes the code harder to read without knowing the definition of the macrot itself. Having to know the _definition_ of a macro to understand how it works violates the usual paradigm of having a fixed API/contract independent of the implementation.

Cheers
Detlev

It's like manually inflatable airbags -- people will never
think to use it in time to actually get any help from it.
             -- Miles Bader in <[email protected]>
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

Hi Wolfgang,

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);

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

Dear Simon Glass,

We don't make any progress if you now go back to the starting point. Please feel free to do so, but I don't follow.

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
"All my life I wanted to be someone; I guess I should have been  more
specific."                                              - Jane Wagner

Hi Wolfgang,

I think I responded to your point about not being able to search in the source code. I can't really respond to your point about it being obscure as I think that is a matter of opinion. But please let me know if I missed something else from your response.

I also presented two options based around clrsetbits_le32. Does something like the below meet with your approval?

(in a header somewhere:)
#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)

(in the C file:)
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))

Regards,
Simon

Dear Simon Glass,

Yes, and doing so you ignore all previous discussion about portability etc. etc. etc.

Let's stop this discussion here. You don't have any new arguments. We're just wasting our time. My mind is set.

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
How can you tell when sour cream goes bad?

These functions provide access to the high resolution microsecond timer and tidy up a global variable in the code.

---
 arch/arm/cpu/armv7/tegra2/timer.c        |   27 +++++++++++++++++------
 arch/arm/include/asm/arch-tegra2/timer.h |   34 ++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 7 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-tegra2/timer.h

index fb061d0..b69c172 100644
@@ -38,13 +38,12 @@
 #include <common.h>
 #include <asm/io.h>
 #include <asm/arch/tegra2.h>
+#include <asm/arch/timer.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
-struct timerus *timer_base = (struct timerus *)NV_PA_TMRUS_BASE;
-
 /* counter runs at 1MHz */
-#define TIMER_CLK	(1000000)
+#define TIMER_CLK	1000000
 #define TIMER_LOAD_VAL	0xffffffff
 
 /* timer without interrupts */

@@ -67,10 +66,10 @@ void set_timer(ulong t)
 void __udelay(unsigned long usec)
 {
 	long tmo = usec * (TIMER_CLK / 1000) / 1000;
-	unsigned long now, last = readl(&timer_base->cntr_1us);
+	unsigned long now, last = timer_get_us();
 
 	while (tmo > 0) {
-		now = readl(&timer_base->cntr_1us);
+		now = timer_get_us();
 		if (last > now) /* count up timer overflow */
 			tmo -= TIMER_LOAD_VAL - last + now;
 		else

@@ -82,7 +81,7 @@ void __udelay(unsigned long usec)
 void reset_timer_masked(void)
 {
 	/* reset time, capture current incrementer value time */
-	gd->lastinc = readl(&timer_base->cntr_1us) / (TIMER_CLK/CONFIG_SYS_HZ);
+	gd->lastinc = timer_get_us() / (TIMER_CLK/CONFIG_SYS_HZ);
 	gd->tbl = 0;		/* start "advancing" time stamp from 0 */
 }
 

@@ -91,7 +90,7 @@ ulong get_timer_masked(void)
 	ulong now;
 
 	/* current tick value */
-	now = readl(&timer_base->cntr_1us) / (TIMER_CLK / CONFIG_SYS_HZ);
+	now = timer_get_us() / (TIMER_CLK / CONFIG_SYS_HZ);
 
 	if (now >= gd->lastinc)	/* normal mode (non roll) */
 		/* move stamp forward with absolute diff ticks */

@@ -120,3 +119,17 @@ ulong get_tbclk(void)
 {
 	return CONFIG_SYS_HZ;
 }
+
+
+unsigned long timer_get_us(void)
+{
+	struct timerus *timer_base = (struct timerus *)NV_PA_TMRUS_BASE;
+
+	return readl(&timer_base->cntr_1us);
+}
+
+unsigned long timer_get_future_us(u32 delay)
+{
+	return timer_get_us() + delay;
+}
+

new file mode 100644
index 0000000..5d5445e
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+/* Tegra2 timer functions */
+
+#ifndef _TEGRA2_TIMER_H
+#define _TEGRA2_TIMER_H
+
+/* returns the current monotonic timer value in microseconds */
+unsigned long timer_get_us(void);
+
+/* returns what the time will likely be some microseconds into the future */
+unsigned long timer_get_future_us(u32 delay);
+
+#endif
+

1.7.3.1

This adds functions to enable/disable clocks and reset to on-chip peripherals.

---
 arch/arm/cpu/armv7/tegra2/Makefile         |    2 +-
 arch/arm/cpu/armv7/tegra2/ap20.c           |   57 ++----
 arch/arm/cpu/armv7/tegra2/clock.c          |  163 +++++++++++++++++
 arch/arm/include/asm/arch-tegra2/clk_rst.h |  100 ++++++-----
 arch/arm/include/asm/arch-tegra2/clock.h   |  264 ++++++++++++++++++++++++++++
 board/nvidia/common/board.c                |   52 ++----
 6 files changed, 518 insertions(+), 120 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/tegra2/clock.c
 create mode 100644 arch/arm/include/asm/arch-tegra2/clock.h

index f1ea915..b35764c 100644
@@ -28,7 +28,7 @@ include $(TOPDIR)/config.mk
 LIB	=  $(obj)lib$(SOC).o
 
 SOBJS	:= lowlevel_init.o
-COBJS	:= ap20.o board.o sys_info.o timer.o
+COBJS	:= ap20.o board.o clock.o sys_info.o timer.o
 
 SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS) $(SOBJS))

1.7.3.1

This adds an enum for each pin and some functions for changing the pin muxing setup.

---
 arch/arm/cpu/armv7/tegra2/Makefile        |    2 +-
 arch/arm/cpu/armv7/tegra2/pinmux.c        |   54 ++++++++++
 arch/arm/include/asm/arch-tegra2/pinmux.h |  156 +++++++++++++++++++++++++++--
 board/nvidia/common/board.c               |   10 +--
 4 files changed, 207 insertions(+), 15 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/tegra2/pinmux.c

index b35764c..f673f03 100644
@@ -28,7 +28,7 @@ include $(TOPDIR)/config.mk
 LIB	=  $(obj)lib$(SOC).o
 
 SOBJS	:= lowlevel_init.o
-COBJS	:= ap20.o board.o clock.o sys_info.o timer.o
+COBJS	:= ap20.o board.o clock.o pinmux.o sys_info.o timer.o
 
 SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS) $(SOBJS))

index 12e79ae..f07328e 100644
@@ -140,19 +140,15 @@ static void pin_mux_uart(void)
 	reg &= 0xFFF0FFFF;	/* IRRX_/IRTX_SEL [19:16] = 00 UARTA */
 	writel(reg, &pmt->pmt_ctl_c);
 
-	reg = readl(&pmt->pmt_tri_a);
-	reg &= ~Z_IRRX;		/* Z_IRRX = normal (0) */
-	reg &= ~Z_IRTX;		/* Z_IRTX = normal (0) */
-	writel(reg, &pmt->pmt_tri_a);
+	pinmux_tristate_disable(PIN_IRRX);
+	pinmux_tristate_disable(PIN_IRTX);
 #endif	/* CONFIG_TEGRA2_ENABLE_UARTA */
 #if defined(CONFIG_TEGRA2_ENABLE_UARTD)
 	reg = readl(&pmt->pmt_ctl_b);
 	reg &= 0xFFFFFFF3;	/* GMC_SEL [3:2] = 00, UARTD */
 	writel(reg, &pmt->pmt_ctl_b);
 
-	reg = readl(&pmt->pmt_tri_a);
-	reg &= ~Z_GMC;		/* Z_GMC = normal (0) */
-	writel(reg, &pmt->pmt_tri_a);
+	pinmux_tristate_disable(PIN_GMC);
 #endif	/* CONFIG_TEGRA2_ENABLE_UARTD */
 }
 

1.7.3.1

---
 arch/arm/cpu/armv7/tegra2/ap20.c           |   47 +++++++++-------------------
 arch/arm/include/asm/arch-tegra2/clk_rst.h |   39 ++---------------------
 board/nvidia/common/board.c                |   13 ++++---
 3 files changed, 25 insertions(+), 74 deletions(-)

index a9bfd6a..7da00cd 100644
@@ -24,6 +24,7 @@
 #include "ap20.h"
 #include <asm/io.h>
 #include <asm/arch/tegra2.h>
+#include <asm/arch/bitfield.h>
 #include <asm/arch/clk_rst.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/pmc.h>

@@ -40,23 +41,22 @@ void init_pllx(void)
 	u32 reg;
 
 	/* If PLLX is already enabled, just return */
-	reg = readl(&pll->pll_base);
-	if (reg & PLL_ENABLE_BIT)
+	if (bf_readl(PLL_ENABLE, &pll->pll_base))
 		return;
 
 	/* Set PLLX_MISC */
-	reg = CPCON;				/* CPCON[11:8]  = 0001 */
+	reg = bf_pack(PLL_CPCON, 1);
 	writel(reg, &pll->pll_misc);
 
 	/* Use 12MHz clock here */
-	reg = (PLL_BYPASS_BIT | PLL_DIVM_VALUE);
-	reg |= (1000 << 8);			/* DIVN = 0x3E8 */
+	reg = bf_pack(PLL_BYPASS, 1) | bf_pack(PLL_DIVM, 12);
+	reg |= bf_pack(PLL_DIVN, 1000);
 	writel(reg, &pll->pll_base);
 
-	reg |= PLL_ENABLE_BIT;
+	reg |= bf_pack(PLL_ENABLE, 1);
 	writel(reg, &pll->pll_base);
 
-	reg &= ~PLL_BYPASS_BIT;
+	reg &= ~bf_mask(PLL_BYPASS);
 	writel(reg, &pll->pll_base);
 }
 

@@ -90,17 +90,12 @@ static void enable_cpu_clock(int enable)
 	 * always stop the clock to CPU 1.
 	 */
 	clk = readl(&clkrst->crc_clk_cpu_cmplx);
-	clk |= CPU1_CLK_STP;
-
-	if (enable) {
-		/* Unstop the CPU clock */
-		clk &= ~CPU0_CLK_STP;
-	} else {
-		/* Stop the CPU clock */
-		clk |= CPU0_CLK_STP;
-	}
+	clk |= bf_pack(CPU1_CLK_STP, 1);
 
+	/* Stop/Unstop the CPU clock */
+	bf_update(CPU0_CLK_STP, clk, enable == 0);
 	writel(clk, &clkrst->crc_clk_cpu_cmplx);
+
 	clock_enable(PERIPH_ID_CPU);
 }
 

@@ -176,9 +171,6 @@ static void enable_cpu_power_rail(void)
 
 static void reset_A9_cpu(int reset)
 {
-	struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
-	u32 cpu;
-
 	/*
 	* NOTE:  Regardless of whether the request is to hold the CPU in reset
 	*        or take it out of reset, every processor in the CPU complex

@@ -187,19 +179,10 @@ static void reset_A9_cpu(int reset)
 	*        are multiple processors in the CPU complex.
 	*/
 
-	/* Hold CPU 1 in reset */
-	cpu = SET_DBGRESET1 | SET_DERESET1 | SET_CPURESET1;
-	writel(cpu, &clkrst->crc_cpu_cmplx_set);
-
-	if (reset) {
-		/* Now place CPU0 into reset */
-		cpu |= SET_DBGRESET0 | SET_DERESET0 | SET_CPURESET0;
-		writel(cpu, &clkrst->crc_cpu_cmplx_set);
-	} else {
-		/* Take CPU0 out of reset */
-		cpu = CLR_DBGRESET0 | CLR_DERESET0 | CLR_CPURESET0;
-		writel(cpu, &clkrst->crc_cpu_cmplx_clr);
-	}
+	/* Hold CPU 1 in reset, and CPU 0 if asked */
+	reset_cmplx_set_enable(1, crc_rst_cpu | crc_rst_de | crc_rst_debug, 1);
+	reset_cmplx_set_enable(0, crc_rst_cpu | crc_rst_de | crc_rst_debug,
+			       reset);
 
 	/* Enable/Disable master CPU reset */
 	reset_set_enable(PERIPH_ID_CPU, reset);

index f51300e..a8c6fb3 100644
@@ -141,42 +141,9 @@ struct clk_rst_ctlr {
 	uint crc_cpu_cmplx_clr;		/* _CPU_CMPLX_CLR_0,	0x344 */
 };
 
-#define PLL_BYPASS_BIT		(1 << 31)
-#define PLL_ENABLE_BIT		(1 << 30)
-#define PLL_BASE_OVRRIDE_BIT	(1 << 28)
-#define PLL_DIVP_VALUE		(1 << 20)	/* post divider, b22:20 */
-#define PLL_DIVM_VALUE		0x0C		/* input divider, b4:0 */
-
-#define SWR_UARTD_RST		(1 << 1)
-#define CLK_ENB_UARTD		(1 << 1)
-#define SWR_UARTA_RST		(1 << 6)
-#define CLK_ENB_UARTA		(1 << 6)
-
-#define SWR_CPU_RST		(1 << 0)
-#define CLK_ENB_CPU		(1 << 0)
-#define SWR_CSITE_RST		(1 << 9)
-#define CLK_ENB_CSITE		(1 << 9)
-
-#define SET_CPURESET0		(1 << 0)
-#define SET_DERESET0		(1 << 4)
-#define SET_DBGRESET0		(1 << 12)
-
-#define SET_CPURESET1		(1 << 1)
-#define SET_DERESET1		(1 << 5)
-#define SET_DBGRESET1		(1 << 13)
-
-#define CLR_CPURESET0		(1 << 0)
-#define CLR_DERESET0		(1 << 4)
-#define CLR_DBGRESET0		(1 << 12)
-
-#define CLR_CPURESET1		(1 << 1)
-#define CLR_DERESET1		(1 << 5)
-#define CLR_DBGRESET1		(1 << 13)
-
-#define CPU0_CLK_STP		(1 << 8)
-#define CPU1_CLK_STP		(1 << 9)
-
-#define CPCON			(1 << 8)
+/* CLK_RST_CONTROLLER_CLK_CPU_CMPLX_0 */
+#define CPU1_CLK_STP_RANGE	9 : 9
+#define CPU0_CLK_STP_RANGE	8 : 8
 
 /* CLK_RST_CONTROLLER_PLLx_BASE_0 */
 #define PLL_BYPASS_RANGE	31 : 31

index f07328e..ac2e3f8 100644
@@ -24,6 +24,7 @@
 #include <common.h>
 #include <ns16550.h>
 #include <asm/io.h>
+#include <asm/arch/bitfield.h>
 #include <asm/arch/tegra2.h>
 #include <asm/arch/sys_proto.h>
 

@@ -78,20 +79,20 @@ static void clock_init_uart(void)
 	u32 reg;
 
 	reg = readl(&pll->pll_base);
-	if (!(reg & PLL_BASE_OVRRIDE_BIT)) {
+	if (!(reg & bf_mask(PLL_BASE_OVRRIDE))) {
 		/* Override pllp setup for 216MHz operation. */
-		reg = (PLL_BYPASS_BIT | PLL_BASE_OVRRIDE_BIT | PLL_DIVP_VALUE);
-		reg |= (((NVRM_PLLP_FIXED_FREQ_KHZ/500) << 8) | PLL_DIVM_VALUE);
+		reg = bf_mask(PLL_BYPASS) | bf_mask(PLL_BASE_OVRRIDE) |
+			bf_pack(PLL_DIVP, 1) | bf_pack(PLL_DIVM, 0xc);
+		reg |= bf_pack(PLL_DIVN, NVRM_PLLP_FIXED_FREQ_KHZ / 500);
 		writel(reg, &pll->pll_base);
 
-		reg |= PLL_ENABLE_BIT;
+		reg |= bf_mask(PLL_ENABLE);
 		writel(reg, &pll->pll_base);
 
-		reg &= ~PLL_BYPASS_BIT;
+		reg &= ~bf_mask(PLL_BYPASS);
 		writel(reg, &pll->pll_base);
 	}
 
-	/* Now do the UART reset/clock enable */
 #if defined(CONFIG_TEGRA2_ENABLE_UARTA)
 	/* Assert UART reset and enable clock */
 	reset_set_enable(PERIPH_ID_UART1, 1);

1.7.3.1

Dear Simon Glass,

They may appear desirable for you, but after carefully reading this code again, and another time, I still have no doubts that I should reject this code.

There are two big issues with it:

- It is non-standard. I don't see in which way this code would be an improvement over the existing standard macros we already have.

- It is unportable (being based on the notion of bit numbers and bit fields which are both compiler and architecture dependent).

In a previous message I already rejected patch no. 1 of this series; I understand the other ones depend on that code, so please consider them rejected, too.

Please use the existing standard macros instead.

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
A man either lives life as it happens to him, meets  it  head-on  and
licks it, or he turns his back on it and starts to wither away.
	-- Dr. Boyce, "The Menagerie" ("The Cage"), stardate unknown