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

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

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


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