[U-Boot] [PATCH 0/5] 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 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
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
Signed-off-by: Simon Glass sjg@chromium.org --- 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
diff --git a/arch/arm/include/asm/arch-tegra2/bitfield.h b/arch/arm/include/asm/arch-tegra2/bitfield.h new file mode 100644 index 0000000..6a1bbfa --- /dev/null +++ b/arch/arm/include/asm/arch-tegra2/bitfield.h @@ -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 diff --git a/test/Makefile b/test/Makefile new file mode 100644 index 0000000..6d8c6a7 --- /dev/null +++ b/test/Makefile @@ -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." diff --git a/test/bitfield.c b/test/bitfield.c new file mode 100644 index 0000000..5fc381d --- /dev/null +++ b/test/bitfield.c @@ -0,0 +1,230 @@ +/* + * 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 + */ + +/* + * Bitfield test routines + * + */ + +#include <stdio.h> + +#include "bitfield.h" + + +#define FULL_RANGE 31 : 0 +#define HIGH_RANGE 31 : 30 +#define MID_RANGE 23 : 16 +#define LOW_RANGE 2 : 0 +#define HIGH_SINGLE_RANGE 31 : 31 +#define MID_SINGLE_RANGE 16 : 16 +#define LOW_SINGLE_RANGE 0 : 0 + +static int test_count; + +#ifdef DEBUG +#define assert(x) \ + ({ test_count++; \ + if (!(x)) \ + printf("Assertion failure '%s' %s line %d\n", \ + #x, __FILE__, __LINE__) }) +#define asserteq(x, y) \ + ({ int _x = x; int _y = y; test_count++; \ + if (_x != _y) \ + printf("Assertion failure at %s:%d: '%s' %#x != '%s' %#x\n", \ + __FILE__, __LINE__, #x, _x, #y, _y); }) +#else +#define assert(x) (test_count++) +#define asserteq(x, y) (test_count++) +#endif + + +static void test_low_high(void) +{ + asserteq(BITFIELD_HIGHBIT(FULL_RANGE), 31); + asserteq(BITFIELD_LOWBIT(FULL_RANGE), 0); + asserteq(BITFIELD_HIGHBIT(HIGH_RANGE), 31); + asserteq(BITFIELD_LOWBIT(HIGH_RANGE), 30); + asserteq(BITFIELD_HIGHBIT(MID_RANGE), 23); + asserteq(BITFIELD_LOWBIT(MID_RANGE), 16); + asserteq(BITFIELD_HIGHBIT(LOW_RANGE), 2); + asserteq(BITFIELD_LOWBIT(LOW_RANGE), 0); + asserteq(BITFIELD_HIGHBIT(HIGH_SINGLE_RANGE), 31); + asserteq(BITFIELD_LOWBIT(HIGH_SINGLE_RANGE), 31); + asserteq(BITFIELD_HIGHBIT(MID_SINGLE_RANGE), 16); + asserteq(BITFIELD_LOWBIT(MID_SINGLE_RANGE), 16); + asserteq(BITFIELD_HIGHBIT(LOW_SINGLE_RANGE), 0); + asserteq(BITFIELD_LOWBIT(LOW_SINGLE_RANGE), 0); +} + +static void test_width(void) +{ + asserteq(BITFIELD_WIDTH(FULL_RANGE), 32); + asserteq(BITFIELD_WIDTH(HIGH_RANGE), 2); + asserteq(BITFIELD_WIDTH(MID_RANGE), 8); + asserteq(BITFIELD_WIDTH(LOW_RANGE), 3); + asserteq(BITFIELD_WIDTH(HIGH_SINGLE_RANGE), 1); + asserteq(BITFIELD_WIDTH(MID_SINGLE_RANGE), 1); + asserteq(BITFIELD_WIDTH(LOW_SINGLE_RANGE), 1); +} + +static void test_shift(void) +{ + asserteq(bf_shift(FULL), 0); + asserteq(bf_shift(HIGH), 30); + asserteq(bf_shift(MID), 16); + asserteq(bf_shift(LOW), 0); + asserteq(bf_shift(HIGH_SINGLE), 31); + asserteq(bf_shift(MID_SINGLE), 16); + asserteq(bf_shift(LOW_SINGLE), 0); +} + +static void test_rawmask(void) +{ + asserteq(bf_rawmask(FULL), 0xffffffffU); + asserteq(bf_rawmask(HIGH), 0x3); + asserteq(bf_rawmask(MID), 0xff); + asserteq(bf_rawmask(LOW), 0x7); + asserteq(bf_rawmask(HIGH_SINGLE), 0x1); + asserteq(bf_rawmask(MID_SINGLE), 0x1); + asserteq(bf_rawmask(LOW_SINGLE), 0x1); +} + +static void test_mask(void) +{ + asserteq(bf_mask(FULL), 0xffffffffU); + asserteq(bf_mask(HIGH), 0xc0000000); + asserteq(bf_mask(MID), 0x00ff0000); + asserteq(bf_mask(LOW), 0x7); + asserteq(bf_mask(HIGH_SINGLE), 0x80000000U); + asserteq(bf_mask(MID_SINGLE), 0x00010000); + asserteq(bf_mask(LOW_SINGLE), 0x1); +} + +static void test_unpack(void) +{ + asserteq(bf_unpack(FULL, 0), 0); + asserteq(bf_unpack(FULL, -1U), -1U); + asserteq(bf_unpack(FULL, 0x12345678), 0x12345678); + asserteq(bf_unpack(FULL, 0x87654321), 0x87654321); + asserteq(bf_unpack(FULL, 0xa5a5a5a6), 0xa5a5a5a6); + + asserteq(bf_unpack(HIGH, 0), 0); + asserteq(bf_unpack(HIGH, -1U), 3); + asserteq(bf_unpack(HIGH, 0x12345678), 0); + asserteq(bf_unpack(HIGH, 0x87654321), 2); + asserteq(bf_unpack(HIGH, 0xa5a5a5a6), 2); + + asserteq(bf_unpack(MID, 0), 0); + asserteq(bf_unpack(MID, -1U), 0xff); + asserteq(bf_unpack(MID, 0x12345678), 0x34); + asserteq(bf_unpack(MID, 0x87654321), 0x65); + asserteq(bf_unpack(MID, 0xa5a5a5a6), 0xa5); + + asserteq(bf_unpack(LOW, 0), 0); + asserteq(bf_unpack(LOW, -1U), 7); + asserteq(bf_unpack(LOW, 0x12345678), 0); + asserteq(bf_unpack(LOW, 0x87654321), 1); + asserteq(bf_unpack(LOW, 0xa5a5a5a6), 6); + + asserteq(bf_unpack(HIGH_SINGLE, 0), 0); + asserteq(bf_unpack(HIGH_SINGLE, -1U), 1); + asserteq(bf_unpack(HIGH_SINGLE, 0x12345678), 0); + asserteq(bf_unpack(HIGH_SINGLE, 0x87654321), 1); + asserteq(bf_unpack(HIGH_SINGLE, 0xa5a5a5a6), 1); + + asserteq(bf_unpack(MID_SINGLE, 0), 0); + asserteq(bf_unpack(MID_SINGLE, -1U), 1); + asserteq(bf_unpack(MID_SINGLE, 0x12345678), 0); + asserteq(bf_unpack(MID_SINGLE, 0x87654321), 1); + asserteq(bf_unpack(MID_SINGLE, 0xa5a5a5a6), 1); + + asserteq(bf_unpack(LOW_SINGLE, 0), 0); + asserteq(bf_unpack(LOW_SINGLE, -1U), 1); + asserteq(bf_unpack(LOW_SINGLE, 0x12345678), 0); + asserteq(bf_unpack(LOW_SINGLE, 0x87654321), 1); + asserteq(bf_unpack(LOW_SINGLE, 0xa5a5a5a6), 0); +} + +static void test_pack(void) +{ + asserteq(bf_pack(FULL, 0), 0); + asserteq(bf_pack(FULL, -1U), -1U); + asserteq(bf_pack(FULL, 0x12345678), 0x12345678); + asserteq(bf_pack(FULL, 0x87654321), 0x87654321); + asserteq(bf_pack(FULL, 0xa5a5a5a6), 0xa5a5a5a6); + + asserteq(bf_pack(HIGH, 0), 0); + asserteq(bf_pack(HIGH, -1U), 0xc0000000); + asserteq(bf_pack(HIGH, 0x12345678), 0); + asserteq(bf_pack(HIGH, 0x87654321), 0x40000000); + asserteq(bf_pack(HIGH, 0xa5a5a5a6), 0x80000000); + + asserteq(bf_pack(MID, 0), 0); + asserteq(bf_pack(MID, -1U), 0x00ff0000); + asserteq(bf_pack(MID, 0x12345678), 0x00780000); + asserteq(bf_pack(MID, 0x87654321), 0x00210000); + asserteq(bf_pack(MID, 0xa5a5a5a6), 0x00a60000); + + asserteq(bf_pack(LOW, 0), 0); + asserteq(bf_pack(LOW, -1U), 7); + asserteq(bf_pack(LOW, 0x12345678), 0); + asserteq(bf_pack(LOW, 0x87654321), 1); + asserteq(bf_pack(LOW, 0xa5a5a5a6), 6); + + asserteq(bf_pack(HIGH_SINGLE, 0), 0); + asserteq(bf_pack(HIGH_SINGLE, -1U), 0x80000000u); + asserteq(bf_pack(HIGH_SINGLE, 0x12345678), 0); + asserteq(bf_pack(HIGH_SINGLE, 0x87654321), 0x80000000u); + asserteq(bf_pack(HIGH_SINGLE, 0xa5a5a5a6), 0x00000000u); + + asserteq(bf_pack(MID_SINGLE, 0), 0); + asserteq(bf_pack(MID_SINGLE, -1U), 0x00010000); + asserteq(bf_pack(MID_SINGLE, 0x12345678), 0); + asserteq(bf_pack(MID_SINGLE, 0x87654321), 0x00010000); + asserteq(bf_pack(MID_SINGLE, 0xa5a5a5a6), 0x00000000); + + asserteq(bf_pack(LOW_SINGLE, 0), 0); + asserteq(bf_pack(LOW_SINGLE, -1U), 1); + asserteq(bf_pack(LOW_SINGLE, 0x12345678), 0); + asserteq(bf_pack(LOW_SINGLE, 0x87654321), 1); + asserteq(bf_pack(LOW_SINGLE, 0xa5a5a5a6), 0); +} + +static void test_set(void) +{ +} + +void bf_test(void) +{ + test_low_high(); + test_width(); + test_shift(); + test_rawmask(); + test_unpack(); + test_pack(); +} + +int main(int argc, char *argv[]) +{ + bf_test(); + printf("%d tests run\n", test_count); + return 0; +}
Dear Simon Glass,
In message 1306973675-8411-2-git-send-email-sjg@chromium.org you wrote:
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
Sorry, but this is a highly unportable, and completely unreadable.
Then: bf_unpack(UART_FBCON) - return the value of bits 5:3 (shifted down to bits 2:0)
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.
- Why have bitfield macros?
- Reability
- Maintainability
- Less error prone
It would be nice if this worked, alas it doesn't. At least not as you expect it.
- 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);
- }
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.
- becomes this:
- bitfield_writel(UTMIP_RESET, 1, &usbctlr->susp_ctrl);
- if (id == PERIPH_ID_USB3)
bitfield_writel(UTMIP_PHY_ENB, 1, &usbctlr->susp_ctrl);
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
On Mon, Jun 6, 2011 at 11:50 AM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
[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
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@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
Dear Simon Glass,
In message BANLkTimCGQbih16u7772-uKHb0bUyApbe14mNQ91zhqi1SS62w@mail.gmail.com you wrote:
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.
Sorry, I fail to understand how you envision to use this, and how it would be different from clrsetbits*() ?
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Mon, Jun 6, 2011 at 11:28 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message BANLkTimCGQbih16u7772-uKHb0bUyApbe14mNQ91zhqi1SS62w@mail.gmail.com you wrote:
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.
Sorry, I fail to understand how you envision to use this, and how it would be different from clrsetbits*() ?
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
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@denx.de No one wants war. -- Kirk, "Errand of Mercy", stardate 3201.7
Dear Simon Glass,
In message BANLkTi=+pqk=UQY_=NoXcMabBDm845xcEw@mail.gmail.com you wrote:
#define clrsetfield_le32(bitfield, addr, value) =A0...
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 ma=
sk)
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.
Sorry, I fail to understand how you envision to use this, and how it would be different from =A0clrsetbits*() ?
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.
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
On Tuesday, June 07, 2011 06:06:23 Wolfgang Denk wrote:
#define FIELD_VAL(x) (x << 16) #define FIELD_MASK FIELD_VAL(0xF)
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
Hi Wolfgang,
On Tue, Jun 7, 2011 at 3:06 AM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message BANLkTi=+pqk=UQY_=NoXcMabBDm845xcEw@mail.gmail.com you wrote:
#define clrsetfield_le32(bitfield, addr, value) =A0...
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 ma=
sk)
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.
Sorry, I fail to understand how you envision to use this, and how it would be different from =A0clrsetbits*() ?
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.
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));
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 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);
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
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@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
Dear Simon Glass,
In message BANLkTimN4NVD43B2Le7_ci9Picjd6pmuXA@mail.gmail.com you wrote:
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 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
Dear Simon Glass,
In message BANLkTimN4NVD43B2Le7_ci9Picjd6pmuXA@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.
You consider this macro helpful, I consider it obscure, and that's why I don't want to have it.
Best regards,
Wolfgang Denk
Hi,
Dear Simon Glass,
In message BANLkTimN4NVD43B2Le7_ci9Picjd6pmuXA@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.
You consider this macro helpful, I consider it obscure, and that's why I don't want to have it.
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
Hi Wolfgang,
On Tue, Jun 7, 2011 at 1:00 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message BANLkTimN4NVD43B2Le7_ci9Picjd6pmuXA@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@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
Dear Simon Glass,
In message BANLkTincWYC+E+_yo+3ZKhUL6ZH_x_neLv1LaUnMxJyp7JN8tg@mail.gmail.com you wrote:
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):
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
On Wed, Jun 8, 2011 at 12:21 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message BANLkTincWYC+E+_yo+3ZKhUL6ZH_x_neLv1LaUnMxJyp7JN8tg@mail.gmail.com you wrote:
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):
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.
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
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@denx.de "All my life I wanted to be someone; I guess I should have been more specific." - Jane Wagner
Dear Simon Glass,
In message BANLkTinCdWfeasmdHkj6MM90k68rgJfef1u2MQ0OmN1DXwdGhw@mail.gmail.com you wrote:
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
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
These functions provide access to the high resolution microsecond timer and tidy up a global variable in the code.
Signed-off-by: Simon Glass sjg@chromium.org --- 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
diff --git a/arch/arm/cpu/armv7/tegra2/timer.c b/arch/arm/cpu/armv7/tegra2/timer.c index fb061d0..b69c172 100644 --- a/arch/arm/cpu/armv7/tegra2/timer.c +++ b/arch/arm/cpu/armv7/tegra2/timer.c @@ -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; +} + diff --git a/arch/arm/include/asm/arch-tegra2/timer.h b/arch/arm/include/asm/arch-tegra2/timer.h new file mode 100644 index 0000000..5d5445e --- /dev/null +++ b/arch/arm/include/asm/arch-tegra2/timer.h @@ -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 +
This adds functions to enable/disable clocks and reset to on-chip peripherals.
Signed-off-by: Simon Glass sjg@chromium.org --- 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
diff --git a/arch/arm/cpu/armv7/tegra2/Makefile b/arch/arm/cpu/armv7/tegra2/Makefile index f1ea915..b35764c 100644 --- a/arch/arm/cpu/armv7/tegra2/Makefile +++ b/arch/arm/cpu/armv7/tegra2/Makefile @@ -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)) diff --git a/arch/arm/cpu/armv7/tegra2/ap20.c b/arch/arm/cpu/armv7/tegra2/ap20.c index 60dd5df..a9bfd6a 100644 --- a/arch/arm/cpu/armv7/tegra2/ap20.c +++ b/arch/arm/cpu/armv7/tegra2/ap20.c @@ -25,6 +25,7 @@ #include <asm/io.h> #include <asm/arch/tegra2.h> #include <asm/arch/clk_rst.h> +#include <asm/arch/clock.h> #include <asm/arch/pmc.h> #include <asm/arch/pinmux.h> #include <asm/arch/scu.h> @@ -35,33 +36,34 @@ u32 s_first_boot = 1; void init_pllx(void) { struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + struct clk_pll *pll = &clkrst->crc_pll[CLOCK_PLL_ID_XCPU]; u32 reg;
/* If PLLX is already enabled, just return */ - reg = readl(&clkrst->crc_pllx_base); - if (reg & PLL_ENABLE) + reg = readl(&pll->pll_base); + if (reg & PLL_ENABLE_BIT) return;
/* Set PLLX_MISC */ reg = CPCON; /* CPCON[11:8] = 0001 */ - writel(reg, &clkrst->crc_pllx_misc); + writel(reg, &pll->pll_misc);
/* Use 12MHz clock here */ - reg = (PLL_BYPASS | PLL_DIVM); + reg = (PLL_BYPASS_BIT | PLL_DIVM_VALUE); reg |= (1000 << 8); /* DIVN = 0x3E8 */ - writel(reg, &clkrst->crc_pllx_base); + writel(reg, &pll->pll_base);
- reg |= PLL_ENABLE; - writel(reg, &clkrst->crc_pllx_base); + reg |= PLL_ENABLE_BIT; + writel(reg, &pll->pll_base);
- reg &= ~PLL_BYPASS; - writel(reg, &clkrst->crc_pllx_base); + reg &= ~PLL_BYPASS_BIT; + writel(reg, &pll->pll_base); }
static void enable_cpu_clock(int enable) { struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; - u32 reg, clk; + u32 clk;
/* * NOTE: @@ -83,10 +85,6 @@ static void enable_cpu_clock(int enable) writel(SUPER_CCLK_DIVIDER, &clkrst->crc_super_cclk_div); }
- /* Fetch the register containing the main CPU complex clock enable */ - reg = readl(&clkrst->crc_clk_out_enb_l); - reg |= CLK_ENB_CPU; - /* * Read the register containing the individual CPU clock enables and * always stop the clock to CPU 1. @@ -103,7 +101,7 @@ static void enable_cpu_clock(int enable) }
writel(clk, &clkrst->crc_clk_cpu_cmplx); - writel(reg, &clkrst->crc_clk_out_enb_l); + clock_enable(PERIPH_ID_CPU); }
static int is_cpu_powered(void) @@ -179,7 +177,7 @@ 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 reg, cpu; + u32 cpu;
/* * NOTE: Regardless of whether the request is to hold the CPU in reset @@ -193,44 +191,27 @@ static void reset_A9_cpu(int reset) cpu = SET_DBGRESET1 | SET_DERESET1 | SET_CPURESET1; writel(cpu, &clkrst->crc_cpu_cmplx_set);
- reg = readl(&clkrst->crc_rst_dev_l); if (reset) { /* Now place CPU0 into reset */ cpu |= SET_DBGRESET0 | SET_DERESET0 | SET_CPURESET0; writel(cpu, &clkrst->crc_cpu_cmplx_set); - - /* Enable master CPU reset */ - reg |= SWR_CPU_RST; } else { /* Take CPU0 out of reset */ cpu = CLR_DBGRESET0 | CLR_DERESET0 | CLR_CPURESET0; writel(cpu, &clkrst->crc_cpu_cmplx_clr); - - /* Disable master CPU reset */ - reg &= ~SWR_CPU_RST; }
- writel(reg, &clkrst->crc_rst_dev_l); + /* Enable/Disable master CPU reset */ + reset_set_enable(PERIPH_ID_CPU, reset); }
static void clock_enable_coresight(int enable) { struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; - u32 rst, clk, src; - - rst = readl(&clkrst->crc_rst_dev_u); - clk = readl(&clkrst->crc_clk_out_enb_u); - - if (enable) { - rst &= ~SWR_CSITE_RST; - clk |= CLK_ENB_CSITE; - } else { - rst |= SWR_CSITE_RST; - clk &= ~CLK_ENB_CSITE; - } + u32 rst, src;
- writel(clk, &clkrst->crc_clk_out_enb_u); - writel(rst, &clkrst->crc_rst_dev_u); + clock_set_enable(PERIPH_ID_CORESIGHT, enable); + reset_set_enable(PERIPH_ID_CORESIGHT, !enable);
if (enable) { /* diff --git a/arch/arm/cpu/armv7/tegra2/clock.c b/arch/arm/cpu/armv7/tegra2/clock.c new file mode 100644 index 0000000..4f73d5e --- /dev/null +++ b/arch/arm/cpu/armv7/tegra2/clock.c @@ -0,0 +1,163 @@ +/* + * 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 Clock control functions */ + +#include <asm/io.h> +#include <asm/arch/bitfield.h> +#include <asm/arch/clk_rst.h> +#include <asm/arch/clock.h> +#include <asm/arch/timer.h> +#include <asm/arch/tegra2.h> +#include <common.h> + +#ifdef DEBUG +#define assert(x) \ + ({ if (!(x)) printf("Assertion failure '%s' %s line %d\n", \ + #x, __FILE__, __LINE__); }) +#else +#define assert(x) +#endif + +/* + * Get the oscillator frequency, from the corresponding hardware configuration + * field. + */ +enum clock_osc_freq clock_get_osc_freq(void) +{ + struct clk_rst_ctlr *clkrst = + (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + u32 reg; + + reg = readl(&clkrst->crc_osc_ctrl); + return bf_unpack(OSC_FREQ, reg); +} + +unsigned long clock_start_pll(enum clock_pll_id clkid, u32 divm, u32 divn, + u32 divp, u32 cpcon, u32 lfcon) +{ + struct clk_rst_ctlr *clkrst = + (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + u32 data; + struct clk_pll *pll; + + assert(clock_pll_id_isvalid(clkid)); + pll = &clkrst->crc_pll[clkid]; + + /* + * We cheat by treating all PLL (except PLLU) in the same fashion. + * This works only because: + * - same fields are always mapped at same offsets, except DCCON + * - DCCON is always 0, doesn't conflict + * - M,N, P of PLLP values are ignored for PLLP + */ + + data = bf_pack(PLL_CPCON, cpcon) | + bf_pack(PLL_LFCON, lfcon); + writel(data, &pll->pll_misc); + + data = bf_pack(PLL_DIVM, divm) | + bf_pack(PLL_DIVN, divn) | + bf_pack(PLL_BYPASS, 0) | + bf_pack(PLL_ENABLE, 1); + + if (clkid == CLOCK_PLL_ID_USB) + data |= bf_pack(PLL_VCO_FREQ, divp); + else + data |= bf_pack(PLL_DIVP, divp); + writel(data, &pll->pll_base); + + /* calculate the stable time */ + return timer_get_future_us(CLOCK_PLL_STABLE_DELAY_US); +} + +void clock_set_enable(enum periph_id periph_id, int enable) +{ + struct clk_rst_ctlr *clkrst = + (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + u32 *clk = &clkrst->crc_clk_out_enb[PERIPH_REG(periph_id)]; + u32 reg; + + /* Enable/disable the clock to this peripheral */ + assert(clock_periph_id_isvalid(periph_id)); + reg = readl(clk); + if (enable) + reg |= PERIPH_MASK(periph_id); + else + reg &= ~PERIPH_MASK(periph_id); + writel(reg, clk); +} + +void clock_enable(enum periph_id clkid) +{ + clock_set_enable(clkid, 1); +} + +void clock_disable(enum periph_id clkid) +{ + clock_set_enable(clkid, 0); +} + +void reset_set_enable(enum periph_id periph_id, int enable) +{ + struct clk_rst_ctlr *clkrst = + (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + u32 *reset = &clkrst->crc_rst_dev[PERIPH_REG(periph_id)]; + u32 reg; + + /* Enable/disable reset to the peripheral */ + assert(clock_periph_id_isvalid(periph_id)); + reg = readl(reset); + if (enable) + reg |= PERIPH_MASK(periph_id); + else + reg &= ~PERIPH_MASK(periph_id); + writel(reg, reset); +} + +void reset_periph(enum periph_id periph_id, int us_delay) +{ + /* Put peripheral into reset */ + reset_set_enable(periph_id, 1); + udelay(us_delay); + + /* Remove reset */ + reset_set_enable(periph_id, 0); + + udelay(us_delay); +} + +void reset_cmplx_set_enable(int cpu, int which, int reset) +{ + struct clk_rst_ctlr *clkrst = + (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + u32 mask; + + /* Form the mask, which depends on the cpu chosen. Tegra2 has 2 */ + assert(cpu >= 0 && cpu < 2); + mask = which << cpu; + + /* either enable or disable those reset for that CPU */ + if (reset) + writel(mask, &clkrst->crc_cpu_cmplx_set); + else + writel(mask, &clkrst->crc_cpu_cmplx_clr); +} diff --git a/arch/arm/include/asm/arch-tegra2/clk_rst.h b/arch/arm/include/asm/arch-tegra2/clk_rst.h index bd8ad2c..f51300e 100644 --- a/arch/arm/include/asm/arch-tegra2/clk_rst.h +++ b/arch/arm/include/asm/arch-tegra2/clk_rst.h @@ -24,15 +24,35 @@ #ifndef _CLK_RST_H_ #define _CLK_RST_H_
+ +/* PLL registers - there are several PLLs in the clock controller */ +struct clk_pll { + uint pll_base; /* the control register */ + uint pll_out; /* output control */ + uint reserved; + uint pll_misc; /* other misc things */ +}; + +/* PLL registers - there are several PLLs in the clock controller */ +struct clk_pll_simple { + uint pll_base; /* the control register */ + uint pll_misc; /* other misc things */ +}; + +/* + * Most PLLs use the clk_pll structure, but some have a simpler two-member + * structure for which we use clk_pll_simple. The reason for this non- + * othogonal setup is not stated. + */ +#define TEGRA_CLK_PLLS 6 +#define TEGRA_CLK_SIMPLE_PLLS 3 /* Number of simple PLLs */ +#define TEGRA_CLK_REGS 3 /* Number of clock enable registers */ + /* Clock/Reset Controller (CLK_RST_CONTROLLER_) regs */ struct clk_rst_ctlr { - uint crc_rst_src; /* _RST_SOURCE_0, 0x00 */ - uint crc_rst_dev_l; /* _RST_DEVICES_L_0, 0x04 */ - uint crc_rst_dev_h; /* _RST_DEVICES_H_0, 0x08 */ - uint crc_rst_dev_u; /* _RST_DEVICES_U_0, 0x0C */ - uint crc_clk_out_enb_l; /* _CLK_OUT_ENB_L_0, 0x10 */ - uint crc_clk_out_enb_h; /* _CLK_OUT_ENB_H_0, 0x14 */ - uint crc_clk_out_enb_u; /* _CLK_OUT_ENB_U_0, 0x18 */ + uint crc_rst_src; /* _RST_SOURCE_0,0x00 */ + uint crc_rst_dev[TEGRA_CLK_REGS]; /* _RST_DEVICES_L/H/U_0 */ + uint crc_clk_out_enb[TEGRA_CLK_REGS]; /* _CLK_OUT_ENB_L/H/U_0 */ uint crc_reserved0; /* reserved_0, 0x1C */ uint crc_cclk_brst_pol; /* _CCLK_BURST_POLICY_0,0x20 */ uint crc_super_cclk_div; /* _SUPER_CCLK_DIVIDER_0,0x24 */ @@ -52,44 +72,11 @@ struct clk_rst_ctlr { uint crc_osc_freq_det_stat; /* _OSC_FREQ_DET_STATUS_0,0x5C */ uint crc_reserved2[8]; /* reserved_2[8], 0x60-7C */
- uint crc_pllc_base; /* _PLLC_BASE_0, 0x80 */ - uint crc_pllc_out; /* _PLLC_OUT_0, 0x84 */ - uint crc_reserved3; /* reserved_3, 0x88 */ - uint crc_pllc_misc; /* _PLLC_MISC_0, 0x8C */ + struct clk_pll crc_pll[TEGRA_CLK_PLLS]; /* PLLs from 0x80 to 0xdc */
- uint crc_pllm_base; /* _PLLM_BASE_0, 0x90 */ - uint crc_pllm_out; /* _PLLM_OUT_0, 0x94 */ - uint crc_reserved4; /* reserved_4, 0x98 */ - uint crc_pllm_misc; /* _PLLM_MISC_0, 0x9C */ + /* PLLs from 0xe0 to 0xf4 */ + struct clk_pll_simple crc_pll_simple[TEGRA_CLK_SIMPLE_PLLS];
- uint crc_pllp_base; /* _PLLP_BASE_0, 0xA0 */ - uint crc_pllp_outa; /* _PLLP_OUTA_0, 0xA4 */ - uint crc_pllp_outb; /* _PLLP_OUTB_0, 0xA8 */ - uint crc_pllp_misc; /* _PLLP_MISC_0, 0xAC */ - - uint crc_plla_base; /* _PLLA_BASE_0, 0xB0 */ - uint crc_plla_out; /* _PLLA_OUT_0, 0xB4 */ - uint crc_reserved5; /* reserved_5, 0xB8 */ - uint crc_plla_misc; /* _PLLA_MISC_0, 0xBC */ - - uint crc_pllu_base; /* _PLLU_BASE_0, 0xC0 */ - uint crc_reserved6; /* _reserved_6, 0xC4 */ - uint crc_reserved7; /* _reserved_7, 0xC8 */ - uint crc_pllu_misc; /* _PLLU_MISC_0, 0xCC */ - - uint crc_plld_base; /* _PLLD_BASE_0, 0xD0 */ - uint crc_reserved8; /* _reserved_8, 0xD4 */ - uint crc_reserved9; /* _reserved_9, 0xD8 */ - uint crc_plld_misc; /* _PLLD_MISC_0, 0xDC */ - - uint crc_pllx_base; /* _PLLX_BASE_0, 0xE0 */ - uint crc_pllx_misc; /* _PLLX_MISC_0, 0xE4 */ - - uint crc_plle_base; /* _PLLE_BASE_0, 0xE8 */ - uint crc_plle_misc; /* _PLLE_MISC_0, 0xEC */ - - uint crc_plls_base; /* _PLLS_BASE_0, 0xF0 */ - uint crc_plls_misc; /* _PLLS_MISC_0, 0xF4 */ uint crc_reserved10; /* _reserved_10, 0xF8 */ uint crc_reserved11; /* _reserved_11, 0xFC */
@@ -154,11 +141,11 @@ struct clk_rst_ctlr { uint crc_cpu_cmplx_clr; /* _CPU_CMPLX_CLR_0, 0x344 */ };
-#define PLL_BYPASS (1 << 31) -#define PLL_ENABLE (1 << 30) -#define PLL_BASE_OVRRIDE (1 << 28) -#define PLL_DIVP (1 << 20) /* post divider, b22:20 */ -#define PLL_DIVM 0x0C /* input divider, b4:0 */ +#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) @@ -191,4 +178,21 @@ struct clk_rst_ctlr {
#define CPCON (1 << 8)
+/* CLK_RST_CONTROLLER_PLLx_BASE_0 */ +#define PLL_BYPASS_RANGE 31 : 31 +#define PLL_ENABLE_RANGE 30 : 30 +#define PLL_BASE_OVRRIDE_RANGE 28 : 28 +#define PLL_DIVP_RANGE 22 : 20 +#define PLL_DIVN_RANGE 17 : 8 +#define PLL_DIVM_RANGE 4 : 0 + +/* CLK_RST_CONTROLLER_PLLx_MISC_0 */ +#define PLL_CPCON_RANGE 11 : 8 +#define PLL_LFCON_RANGE 7 : 4 +#define PLLU_VCO_FREQ_RANGE 20 : 20 +#define PLL_VCO_FREQ_RANGE 3 : 0 + +/* CLK_RST_CONTROLLER_OSC_CTRL_0 */ +#define OSC_FREQ_RANGE 31 : 30 + #endif /* CLK_RST_H */ diff --git a/arch/arm/include/asm/arch-tegra2/clock.h b/arch/arm/include/asm/arch-tegra2/clock.h new file mode 100644 index 0000000..81a9f02 --- /dev/null +++ b/arch/arm/include/asm/arch-tegra2/clock.h @@ -0,0 +1,264 @@ +/* + * 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 clock control functions */ + +#ifndef _CLOCK_H + + +/* Set of oscillator frequencies supported in the internal API. */ +enum clock_osc_freq { + /* All in MHz, so 13_0 is 13.0MHz */ + CLOCK_OSC_FREQ_13_0, + CLOCK_OSC_FREQ_19_2, + CLOCK_OSC_FREQ_12_0, + CLOCK_OSC_FREQ_26_0, + + CLOCK_OSC_FREQ_COUNT, +}; + +/* The PLLs supported by the hardware */ +enum clock_pll_id { + CLOCK_PLL_ID_FIRST, + CLOCK_PLL_ID_CGENERAL = CLOCK_PLL_ID_FIRST, + CLOCK_PLL_ID_MEMORY, + CLOCK_PLL_ID_PERIPH, + CLOCK_PLL_ID_AUDIO, + CLOCK_PLL_ID_USB, + CLOCK_PLL_ID_DISPLAY, + + /* now the simple ones */ + CLOCK_PLL_ID_FIRST_SIMPLE, + CLOCK_PLL_ID_XCPU = CLOCK_PLL_ID_FIRST_SIMPLE, + CLOCK_PLL_ID_EPCI, + CLOCK_PLL_ID_SFROM32KHZ, + + CLOCK_PLL_ID_COUNT, +}; + +/* The clocks supported by the hardware */ +enum periph_id { + PERIPH_ID_FIRST, + + /* Low word: 31:0 */ + PERIPH_ID_CPU = PERIPH_ID_FIRST, + PERIPH_ID_RESERVED1, + PERIPH_ID_RESERVED2, + PERIPH_ID_AC97, + PERIPH_ID_RTC, + PERIPH_ID_TMR, + PERIPH_ID_UART1, + PERIPH_ID_UART2, + + /* 8 */ + PERIPH_ID_GPIO, + PERIPH_ID_SDMMC2, + PERIPH_ID_SPDIF, + PERIPH_ID_I2S1, + PERIPH_ID_I2C1, + PERIPH_ID_NDFLASH, + PERIPH_ID_SDMMC1, + PERIPH_ID_SDMMC4, + + /* 16 */ + PERIPH_ID_TWC, + PERIPH_ID_PWC, + PERIPH_ID_I2S2, + PERIPH_ID_EPP, + PERIPH_ID_VI, + PERIPH_ID_2D, + PERIPH_ID_USBD, + PERIPH_ID_ISP, + + /* 24 */ + PERIPH_ID_3D, + PERIPH_ID_IDE, + PERIPH_ID_DISP2, + PERIPH_ID_DISP1, + PERIPH_ID_HOST1X, + PERIPH_ID_VCP, + PERIPH_ID_RESERVED30, + PERIPH_ID_CACHE2, + + /* Middle word: 63:32 */ + PERIPH_ID_MEM, + PERIPH_ID_AHBDMA, + PERIPH_ID_APBDMA, + PERIPH_ID_RESERVED35, + PERIPH_ID_KBC, + PERIPH_ID_STAT_MON, + PERIPH_ID_PMC, + PERIPH_ID_FUSE, + + /* 40 */ + PERIPH_ID_KFUSE, + PERIPH_ID_SBC1, + PERIPH_ID_SNOR, + PERIPH_ID_SPI1, + PERIPH_ID_SBC2, + PERIPH_ID_XIO, + PERIPH_ID_SBC3, + PERIPH_ID_DVC_I2C, + + /* 48 */ + PERIPH_ID_DSI, + PERIPH_ID_TVO, + PERIPH_ID_MIPI, + PERIPH_ID_HDMI, + PERIPH_ID_CSI, + PERIPH_ID_TVDAC, + PERIPH_ID_I2C2, + PERIPH_ID_UART3, + + /* 56 */ + PERIPH_ID_RESERVED56, + PERIPH_ID_EMC, + PERIPH_ID_USB2, + PERIPH_ID_USB3, + PERIPH_ID_MPE, + PERIPH_ID_VDE, + PERIPH_ID_BSEA, + PERIPH_ID_BSEV, + + /* Upper word 95:64 */ + PERIPH_ID_SPEEDO, + PERIPH_ID_UART4, + PERIPH_ID_UART5, + PERIPH_ID_I2C3, + PERIPH_ID_SBC4, + PERIPH_ID_SDMMC3, + PERIPH_ID_PCIE, + PERIPH_ID_OWR, + + /* 72 */ + PERIPH_ID_AFI, + PERIPH_ID_CORESIGHT, + PERIPH_ID_RESERVED74, + PERIPH_ID_AVPUCQ, + PERIPH_ID_RESERVED76, + PERIPH_ID_RESERVED77, + PERIPH_ID_RESERVED78, + PERIPH_ID_RESERVED79, + + /* 80 */ + PERIPH_ID_RESERVED80, + PERIPH_ID_RESERVED81, + PERIPH_ID_RESERVED82, + PERIPH_ID_RESERVED83, + PERIPH_ID_IRAMA, + PERIPH_ID_IRAMB, + PERIPH_ID_IRAMC, + PERIPH_ID_IRAMD, + + /* 88 */ + PERIPH_ID_CRAM2, + + PERIPH_ID_COUNT, +}; + +/* Converts a clock number to a clock register: 0=L, 1=H, 2=U */ +#define PERIPH_REG(id) ((id) >> 5) + +/* Mask value for a clock (within PERIPH_REG(id)) */ +#define PERIPH_MASK(id) (1 << ((id) & 0x1f)) + +/* return 1 if a PLL ID is in range */ +#define clock_pll_id_isvalid(id) ((id) >= CLOCK_PLL_ID_FIRST && \ + (id) < CLOCK_PLL_ID_COUNT) + +/* return 1 if a peripheral ID is in range */ +#define clock_periph_id_isvalid(id) ((id) >= PERIPH_ID_FIRST && \ + (id) < PERIPH_ID_COUNT) + +/* PLL stabilization delay in usec */ +#define CLOCK_PLL_STABLE_DELAY_US 300 + +/* return the current oscillator clock frequency */ +enum clock_osc_freq clock_get_osc_freq(void); + +/* + * Start PLL using the provided configuration parameters. + * + * @param id clock id + * @param divm input divider + * @param divn feedback divider + * @param divp post divider 2^n + * @param cpcon charge pump setup control + * @param lfcon loop filter setup control + * + * @returns monotonic time in us that the PLL will be stable + */ +unsigned long clock_start_pll(enum clock_pll_id id, u32 divm, u32 divn, + u32 divp, u32 cpcon, u32 lfcon); + +/* + * Enable a clock + * + * @param id clock id + */ +void clock_enable(enum periph_id clkid); + +/* + * Set whether a clock is enabled or disabled. + * + * @param id clock id + * @param enable 1 to enable, 0 to disable + */ +void clock_set_enable(enum periph_id clkid, int enable); + +/* + * Reset a peripheral. This puts it in reset, waits for a delay, then takes + * it out of reset and waits for th delay again. + * + * @param periph_id peripheral to reset + * @param us_delay time to delay in microseconds + */ +void reset_periph(enum periph_id periph_id, int us_delay); + +/* + * Put a peripheral into or out of reset. + * + * @param periph_id peripheral to reset + * @param enable 1 to put into reset, 0 to take out of reset + */ +void reset_set_enable(enum periph_id periph_id, int enable); + + +/* CLK_RST_CONTROLLER_RST_CPU_CMPLX_SET/CLR_0 */ +enum crc_reset_id { + /* Things we can hold in reset for each CPU */ + crc_rst_cpu = 1, + crc_rst_de = 1 << 2, /* What is de? */ + crc_rst_watchdog = 1 << 3, + crc_rst_debug = 1 << 4, +}; + +/* + * Put parts of the CPU complex into or out of reset.\ + * + * @param cpu cpu number (0 or 1 on Tegra2) + * @param which which parts of the complex to affect (OR of crc_reset_id) + * @param reset 1 to assert reset, 0 to de-assert + */ +void reset_cmplx_set_enable(int cpu, int which, int reset); + +#endif + diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c index 3d6c248..12e79ae 100644 --- a/board/nvidia/common/board.c +++ b/board/nvidia/common/board.c @@ -28,6 +28,7 @@ #include <asm/arch/sys_proto.h>
#include <asm/arch/clk_rst.h> +#include <asm/arch/clock.h> #include <asm/arch/pinmux.h> #include <asm/arch/uart.h> #include "board.h" @@ -73,33 +74,28 @@ int timer_init(void) static void clock_init_uart(void) { struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + struct clk_pll *pll = &clkrst->crc_pll[CLOCK_PLL_ID_PERIPH]; u32 reg;
- reg = readl(&clkrst->crc_pllp_base); - if (!(reg & PLL_BASE_OVRRIDE)) { + reg = readl(&pll->pll_base); + if (!(reg & PLL_BASE_OVRRIDE_BIT)) { /* Override pllp setup for 216MHz operation. */ - reg = (PLL_BYPASS | PLL_BASE_OVRRIDE | PLL_DIVP); - reg |= (((NVRM_PLLP_FIXED_FREQ_KHZ/500) << 8) | PLL_DIVM); - writel(reg, &clkrst->crc_pllp_base); + reg = (PLL_BYPASS_BIT | PLL_BASE_OVRRIDE_BIT | PLL_DIVP_VALUE); + reg |= (((NVRM_PLLP_FIXED_FREQ_KHZ/500) << 8) | PLL_DIVM_VALUE); + writel(reg, &pll->pll_base);
- reg |= PLL_ENABLE; - writel(reg, &clkrst->crc_pllp_base); + reg |= PLL_ENABLE_BIT; + writel(reg, &pll->pll_base);
- reg &= ~PLL_BYPASS; - writel(reg, &clkrst->crc_pllp_base); + reg &= ~PLL_BYPASS_BIT; + writel(reg, &pll->pll_base); }
/* Now do the UART reset/clock enable */ #if defined(CONFIG_TEGRA2_ENABLE_UARTA) - /* Assert Reset to UART */ - reg = readl(&clkrst->crc_rst_dev_l); - reg |= SWR_UARTA_RST; /* SWR_UARTA_RST = 1 */ - writel(reg, &clkrst->crc_rst_dev_l); - - /* Enable clk to UART */ - reg = readl(&clkrst->crc_clk_out_enb_l); - reg |= CLK_ENB_UARTA; /* CLK_ENB_UARTA = 1 */ - writel(reg, &clkrst->crc_clk_out_enb_l); + /* Assert UART reset and enable clock */ + reset_set_enable(PERIPH_ID_UART1, 1); + clock_enable(PERIPH_ID_UART1);
/* Enable pllp_out0 to UART */ reg = readl(&clkrst->crc_clk_src_uarta); @@ -110,20 +106,12 @@ static void clock_init_uart(void) udelay(2);
/* De-assert reset to UART */ - reg = readl(&clkrst->crc_rst_dev_l); - reg &= ~SWR_UARTA_RST; /* SWR_UARTA_RST = 0 */ - writel(reg, &clkrst->crc_rst_dev_l); + reset_set_enable(PERIPH_ID_UART1, 0); #endif /* CONFIG_TEGRA2_ENABLE_UARTA */ #if defined(CONFIG_TEGRA2_ENABLE_UARTD) - /* Assert Reset to UART */ - reg = readl(&clkrst->crc_rst_dev_u); - reg |= SWR_UARTD_RST; /* SWR_UARTD_RST = 1 */ - writel(reg, &clkrst->crc_rst_dev_u); - - /* Enable clk to UART */ - reg = readl(&clkrst->crc_clk_out_enb_u); - reg |= CLK_ENB_UARTD; /* CLK_ENB_UARTD = 1 */ - writel(reg, &clkrst->crc_clk_out_enb_u); + /* Assert UART reset and enable clock */ + reset_set_enable(PERIPH_ID_UART4, 1); + clock_enable(PERIPH_ID_UART4);
/* Enable pllp_out0 to UART */ reg = readl(&clkrst->crc_clk_src_uartd); @@ -134,9 +122,7 @@ static void clock_init_uart(void) udelay(2);
/* De-assert reset to UART */ - reg = readl(&clkrst->crc_rst_dev_u); - reg &= ~SWR_UARTD_RST; /* SWR_UARTD_RST = 0 */ - writel(reg, &clkrst->crc_rst_dev_u); + reset_set_enable(PERIPH_ID_UART4, 0); #endif /* CONFIG_TEGRA2_ENABLE_UARTD */ }
This adds an enum for each pin and some functions for changing the pin muxing setup.
Signed-off-by: Simon Glass sjg@chromium.org --- 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
diff --git a/arch/arm/cpu/armv7/tegra2/Makefile b/arch/arm/cpu/armv7/tegra2/Makefile index b35764c..f673f03 100644 --- a/arch/arm/cpu/armv7/tegra2/Makefile +++ b/arch/arm/cpu/armv7/tegra2/Makefile @@ -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)) diff --git a/arch/arm/cpu/armv7/tegra2/pinmux.c b/arch/arm/cpu/armv7/tegra2/pinmux.c new file mode 100644 index 0000000..f00fd7c --- /dev/null +++ b/arch/arm/cpu/armv7/tegra2/pinmux.c @@ -0,0 +1,54 @@ +/* + * 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 pin multiplexing functions */ + +#include <asm/io.h> +#include <asm/arch/bitfield.h> +#include <asm/arch/tegra2.h> +#include <asm/arch/pinmux.h> +#include <common.h> + + +void pinmux_set_tristate(enum pmux_pin pin, int enable) +{ + struct pmux_tri_ctlr *pmt = (struct pmux_tri_ctlr *)NV_PA_APB_MISC_BASE; + u32 *tri = &pmt->pmt_tri[TRISTATE_REG(pin)]; + u32 reg; + + reg = readl(tri); + if (enable) + reg |= TRISTATE_MASK(pin); + else + reg &= ~TRISTATE_MASK(pin); + writel(reg, tri); +} + +void pinmux_tristate_enable(enum pmux_pin pin) +{ + pinmux_set_tristate(pin, 1); +} + +void pinmux_tristate_disable(enum pmux_pin pin) +{ + pinmux_set_tristate(pin, 0); +} + diff --git a/arch/arm/include/asm/arch-tegra2/pinmux.h b/arch/arm/include/asm/arch-tegra2/pinmux.h index 8b4bd8d..d4a0812 100644 --- a/arch/arm/include/asm/arch-tegra2/pinmux.h +++ b/arch/arm/include/asm/arch-tegra2/pinmux.h @@ -24,6 +24,143 @@ #ifndef _PINMUX_H_ #define _PINMUX_H_
+ +/* Pins which we can set to tristate or normal */ +enum pmux_pin { + /* APB_MISC_PP_TRISTATE_REG_A_0 */ + PIN_ATA, + PIN_ATB, + PIN_ATC, + PIN_ATD, + PIN_CDEV1, + PIN_CDEV2, + PIN_CSUS, + PIN_DAP1, + + PIN_DAP2, + PIN_DAP3, + PIN_DAP4, + PIN_DTA, + PIN_DTB, + PIN_DTC, + PIN_DTD, + PIN_DTE, + + PIN_GPU, + PIN_GPV, + PIN_I2CP, + PIN_IRTX, + PIN_IRRX, + PIN_KBCB, + PIN_KBCA, + PIN_PMC, + + PIN_PTA, + PIN_RM, + PIN_KBCE, + PIN_KBCF, + PIN_GMA, + PIN_GMC, + PIN_SDMMC1, + PIN_OWC, + + /* 32: APB_MISC_PP_TRISTATE_REG_B_0 */ + PIN_GME, + PIN_SDC, + PIN_SDD, + PIN_RESERVED0, + PIN_SLXA, + PIN_SLXC, + PIN_SLXD, + PIN_SLXK, + + PIN_SPDI, + PIN_SPDO, + PIN_SPIA, + PIN_SPIB, + PIN_SPIC, + PIN_SPID, + PIN_SPIE, + PIN_SPIF, + + PIN_SPIG, + PIN_SPIH, + PIN_UAA, + PIN_UAB, + PIN_UAC, + PIN_UAD, + PIN_UCA, + PIN_UCB, + + PIN_RESERVED1, + PIN_ATE, + PIN_KBCC, + PIN_RESERVED2, + PIN_RESERVED3, + PIN_GMB, + PIN_GMD, + PIN_DDC, + + /* 64: APB_MISC_PP_TRISTATE_REG_C_0 */ + PIN_LD0, + PIN_LD1, + PIN_LD2, + PIN_LD3, + PIN_LD4, + PIN_LD5, + PIN_LD6, + PIN_LD7, + + PIN_LD8, + PIN_LD9, + PIN_LD10, + PIN_LD11, + PIN_LD12, + PIN_LD13, + PIN_LD14, + PIN_LD15, + + PIN_LD16, + PIN_LD17, + PIN_LHP0, + PIN_LHP1, + PIN_LHP2, + PIN_LVP0, + PIN_LVP1, + PIN_HDINT, + + PIN_LM0, + PIN_LM1, + PIN_LVS, + PIN_LSC0, + PIN_LSC1, + PIN_LSCK, + PIN_LDC, + PIN_LCSN, + + /* 96: APB_MISC_PP_TRISTATE_REG_D_0 */ + PIN_LSPI, + PIN_LSDA, + PIN_LSDI, + PIN_LPW0, + PIN_LPW1, + PIN_LPW2, + PIN_LDI, + PIN_LHS, + + PIN_LPP, + PIN_RESERVED4, + PIN_KBCD, + PIN_GPU7, + PIN_DTF, + PIN_UDA, + PIN_CRTP, + PIN_SDB, +}; + + +#define TEGRA_TRISTATE_REGS 4 + /* APB MISC Pin Mux and Tristate (APB_MISC_PP_) registers */ struct pmux_tri_ctlr { uint pmt_reserved0; /* ABP_MISC_PP_ reserved offset 00 */ @@ -31,10 +168,7 @@ struct pmux_tri_ctlr { uint pmt_strap_opt_a; /* _STRAPPING_OPT_A_0, offset 08 */ uint pmt_reserved2; /* ABP_MISC_PP_ reserved offset 0C */ uint pmt_reserved3; /* ABP_MISC_PP_ reserved offset 10 */ - uint pmt_tri_a; /* _TRI_STATE_REG_A_0, offset 14 */ - uint pmt_tri_b; /* _TRI_STATE_REG_B_0, offset 18 */ - uint pmt_tri_c; /* _TRI_STATE_REG_C_0, offset 1C */ - uint pmt_tri_d; /* _TRI_STATE_REG_D_0, offset 20 */ + uint pmt_tri[TEGRA_TRISTATE_REGS]; /* _TRI_STATE_REG_A/B/C/D_0 14-20 */ uint pmt_cfg_ctl; /* _CONFIG_CTL_0, offset 24 */
uint pmt_reserved[22]; /* ABP_MISC_PP_ reserved offs 28-7C */ @@ -48,8 +182,16 @@ struct pmux_tri_ctlr { uint pmt_ctl_g; /* _PIN_MUX_CTL_G_0, offset 98 */ };
-#define Z_GMC (1 << 29) -#define Z_IRRX (1 << 20) -#define Z_IRTX (1 << 19) +/* Converts a pin number to a tristate register: 0=A, 1=B, 2=C, 3=D */ +#define TRISTATE_REG(id) ((id) >> 5) + +/* Mask value for a tristate (within TRISTATE_REG(id)) */ +#define TRISTATE_MASK(id) (1 << ((id) & 0x1f)) + +/* Set a pin to tristate */ +void pinmux_tristate_enable(enum pmux_pin pin); + +/* Set a pin to normal (non tristate) */ +void pinmux_tristate_disable(enum pmux_pin pin);
#endif /* PINMUX_H */ diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c index 12e79ae..f07328e 100644 --- a/board/nvidia/common/board.c +++ b/board/nvidia/common/board.c @@ -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 */ }
Signed-off-by: Simon Glass sjg@chromium.org --- 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(-)
diff --git a/arch/arm/cpu/armv7/tegra2/ap20.c b/arch/arm/cpu/armv7/tegra2/ap20.c index a9bfd6a..7da00cd 100644 --- a/arch/arm/cpu/armv7/tegra2/ap20.c +++ b/arch/arm/cpu/armv7/tegra2/ap20.c @@ -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); diff --git a/arch/arm/include/asm/arch-tegra2/clk_rst.h b/arch/arm/include/asm/arch-tegra2/clk_rst.h index f51300e..a8c6fb3 100644 --- a/arch/arm/include/asm/arch-tegra2/clk_rst.h +++ b/arch/arm/include/asm/arch-tegra2/clk_rst.h @@ -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 diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c index f07328e..ac2e3f8 100644 --- a/board/nvidia/common/board.c +++ b/board/nvidia/common/board.c @@ -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);
Dear Simon Glass,
In message 1306973675-8411-1-git-send-email-sjg@chromium.org you wrote:
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.
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
participants (4)
-
Detlev Zundel -
Mike Frysinger -
Simon Glass -
Wolfgang Denk