Re: [bootwrapper PATCH 02/13] Add bit-field macros

[Andre Przywara <andre.przywara@arm.com>]

On Tue, 11 Jan 2022 13:06:42 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

Hi Mark,

> Arm architectural documentation typically defines bit-fields as
> `[msb,lsb]` and single-bit fields as `[bit]`. For clarity it would be
> helpful if we could define fields in the same way.
> 
> Add helpers so that we can do so, along with helper to extract/insert
> bit-field values.
> 
> There should be no functional change as a result of this patch.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  include/bits.h | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 include/bits.h
> 
> diff --git a/include/bits.h b/include/bits.h
> new file mode 100644
> index 0000000..8824a38
> --- /dev/null
> +++ b/include/bits.h
> @@ -0,0 +1,33 @@
> +/*
> + * include/bits.h - helpers for bit-field definitions.
> + *
> + * Copyright (C) 2021 ARM Limited. All rights reserved.
> + *
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE.txt file.
> + */
> +#ifndef __BITS_H
> +#define __BITS_H
> +
> +#ifdef __ASSEMBLY__
> +#define UL(x)	x
> +#define ULL(x)	x
> +#else
> +#define UL(x)	x##UL
> +#define ULL(x)	x##ULL
> +#endif
> +
> +#define BITS(msb, lsb) \

The kernel uses GENMASK() for this, should we follow suit here? Both
U-Boot and Trusted Firmware decided to do so, so I consider this some kind
of agreed naming for bitmask generation these days.

> +((~ULL(0) >> (63 - msb)) & (~ULL(0) << lsb))
> +
> +#define BIT(b)	BITS(b, b)
> +
> +#define BITS_LSB(bits)	(__builtin_ffsll(bits) - 1)

Shall there be some comment explaining the functionality and arguments? Or
maybe use "mask" instead of the more ambiguous "bits" name here?
TBH I needed to read the implementation of the next macro to understand
what it does.

> +
> +#define BITS_EXTRACT(val, bits) \

Same here, having BITS_EXTRACT(val, mask) looks more readable to me.

Cheers,
Andre

> +	(((val) & (bits)) >> BITS_LSB(bits))
> +
> +#define BITS_INSERT(bits, val) \
> +	(((val) << BITS_LSB(bits)) & (bits))
> +
> +#endif


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel