[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

Hi Simon,

Le 05/07/2011 18:49, Simon Glass a ?crit :
> Signed-off-by: Simon Glass<sjg@chromium.org>
> ---
> Changes in v2:
> - Removed all bitfield access macros

Not sure I follow you: the added lines below do indeed add bitfield 
access macros, don't they?

>   arch/arm/include/asm/arch-tegra2/bitfield.h |   96 +++++++++++++++++++++++++++
>   1 files changed, 96 insertions(+), 0 deletions(-)
>   create mode 100644 arch/arm/include/asm/arch-tegra2/bitfield.h
>
> 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..494087c
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-tegra2/bitfield.h
> @@ -0,0 +1,96 @@
> +/*
> + * 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
> +
> +/*
> + * Basic macros for easily getting mask and shift values for bit fields on
> + * ARM.
> + *
> + * You use these to reliably create shifts and masks from a bit field
> + * definition. Bit fields are defined like this:
> + *
> + * #define NAME_BITS	MSB : LSB
> + *
> + * where MSB is the most significant bit, and LSB the least sig, bit. This
> + * notation is chosen since it is commonly used in CPU / SOC datasheets.
> + *
> + * For example:
> + *
> + * #define UART_FBCON_BITS  5:3		Bit range for the FBCON field
> + *
> + * Note that if you are using a big-endian machine there is no consistent
> + * notion of big numbers, since bit 3 is a different bit depending on whether
> + * the access is 32-bits or 64-bits. For this reason these macros should not
> + * be used as it would probably be too confusing to have to specify your
> + * access width all the time.
> + *
> + * This defines a bit field extending between bits 3 and 5.
> + *
> + * Then in your header file you can set up the shift and mask like this:
> + *
> + *	 #define UART_FBCON_SHIFT	bf_shift(UART_FBCON)
> + *	 #define UART_FBCON_MASK	bf_mask(UART_FBCON)
> + *
> + * Then you can use these macros in your code (there is no bitfield support
> + * in the C file and these macros MUST NOT be used directly in C code).
> + *
> + * To write, overwriting other fields too:
> + *	writel(3<<  UART_FBCON_SHIFT,&uart->fbcon);
> + *
> + * To read:
> + *	int fbcon = (readl(&uart->fbcon)&  UART_FBCON_MASK)>>
> + *		UART_FBCON_SHIFT;
> + *
> + * To update just this field (for example):
> + *	clrsetbits_le32(&uart->fbcon, UART_FBCON_MASK, 3<<  UART_FBCON_SHIFT);
> + */

What's the benefit of this over directly specifying a shift and mask 
value, e.g. #define UART_FBCON_SHIFT 3 and #define UART_FBCON_MASK 0x7 
and doing shifts and ANDs? I don't see this as simpler or more intuitive.

> +#include "compiler.h"
> +
> +#if __BYTE_ORDER == __LITTLE_ENDIAN
> +
> +/* Returns the bit number of the LSB */
> +#define _LSB(range)	((0 ? range)&  0x1f)
> +
> +/* Returns the bit number of the MSB */
> +#define _MSB(range)	(1 ? range)
> +
> +/* Returns the width of the bitfield (in bits) */
> +#define _BITFIELD_WIDTH(range)	(_MSB(range) - _LSB(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)		_LSB(field)
> +
> +/* Returns the unshifted mask for the field (i.e. LSB of mask is bit 0) */
> +#define bf_rawmask(field)	((1UL<<  _BITFIELD_WIDTH(field)) - 1)
> +
> +/* Returns the mask for a field. Clear these bits to zero the field */
> +#define bf_mask(field)		(bf_rawmask(field)<<  (bf_shift(field)))
> +
> +#endif /* __BYTE_ORDER == __LITTLE_ENDIAN */
> +
> +#endif

Amicalement,
-- 
Albert.