| Message ID | 20220111130653.2331827-3-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanups and improvements | expand |
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
On Tue, Jan 11, 2022 at 02:40:48PM +0000, Andre Przywara wrote: > 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. TBH, I always forget the naming of GENMASK(), and chose `BITS()` to more clearly align with `BIT()`, and also the way the architecture documentation speaks about "bits [msb:lsb]". I'm not wedded to the naming, but IMO `GENMASK()` isn't any better, even if that's what linux uses. Regardless of the specific names, I'd like the single-bit and multi-bit helpers to clearly align naming-wise. For now I'd prefer to stick with `BIT()` and `BITS()`. > > +((~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. If there's any confusion here I think we need comments regardless, since neither `bits` nor `mask` imply contiguity, which is the important factor. I'll add some comments with examples. I'm happy to also rename the `bits` parameter to `mask`. > > + > > +#define BITS_EXTRACT(val, bits) \ > > Same here, having BITS_EXTRACT(val, mask) looks more readable to me. I'll do as above hree, and likewise for the cases below. Thanks, Mark. > > Cheers, > Andre > > > + (((val) & (bits)) >> BITS_LSB(bits)) > > + > > +#define BITS_INSERT(bits, val) \ > > + (((val) << BITS_LSB(bits)) & (bits)) > > + > > +#endif >
On Wed, 12 Jan 2022 14:16:21 +0000 Mark Rutland <mark.rutland@arm.com> wrote: Hi, > On Tue, Jan 11, 2022 at 02:40:48PM +0000, Andre Przywara wrote: > > 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. > > TBH, I always forget the naming of GENMASK(), and chose `BITS()` to more > clearly align with `BIT()`, and also the way the architecture documentation > speaks about "bits [msb:lsb]". > > I'm not wedded to the naming, but IMO `GENMASK()` isn't any better, even if > that's what linux uses. Regardless of the specific names, I'd like the > single-bit and multi-bit helpers to clearly align naming-wise. > > For now I'd prefer to stick with `BIT()` and `BITS()`. Fair enough, seeing that in the code in later patches looked alright, I guess having two arguments sets it apart enough from just BIT. Cheers, Andre > > > +((~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. > > If there's any confusion here I think we need comments regardless, since > neither `bits` nor `mask` imply contiguity, which is the important factor. I'll > add some comments with examples. > > I'm happy to also rename the `bits` parameter to `mask`. > > > > + > > > +#define BITS_EXTRACT(val, bits) \ > > > > Same here, having BITS_EXTRACT(val, mask) looks more readable to me. > > I'll do as above hree, and likewise for the cases below. > > Thanks, > Mark. > > > > > Cheers, > > Andre > > > > > + (((val) & (bits)) >> BITS_LSB(bits)) > > > + > > > +#define BITS_INSERT(bits, val) \ > > > + (((val) << BITS_LSB(bits)) & (bits)) > > > + > > > +#endif > >
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) \ +((~ULL(0) >> (63 - msb)) & (~ULL(0) << lsb)) + +#define BIT(b) BITS(b, b) + +#define BITS_LSB(bits) (__builtin_ffsll(bits) - 1) + +#define BITS_EXTRACT(val, bits) \ + (((val) & (bits)) >> BITS_LSB(bits)) + +#define BITS_INSERT(bits, val) \ + (((val) << BITS_LSB(bits)) & (bits)) + +#endif
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