From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9EDF7C433F5 for ; Fri, 14 Jan 2022 18:14:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=y6AIk1K35ZnATBzkX+WIJvisBzEwBQZRhC/2pBFf4Ek=; b=Ess54+Df9EsYH+ qcqaqBLgVdMDuBJHULKXlLFV6X7X7iMf+8Bw9oqmP6PtzTQYO+LLJQtQrL2U+u4VD6nIqKpGP/caJ WvA7DIf/cu2nz0sYC5Gone1KPjgE+6gWjTPfGdOmGqL6KIwwVepu0L+6Evr1dhNOpU8eH0iwwq4n0 HVOmfO0IWpDQthDhgMc9d4q8fviDrh8lZd0AhczT9KGEc2BXFwXN8QKaSpYyQ+IJTQt9jcu9d3cKd 0/Ka2OhfnNwv5c9F+Xj3yih/GUjnXWXlM9VEIpz/c1rR7SXXZmVa44SXQTkFiJxZHrTYRfyZRZOUk cMKPgXPUBtgCq0sT1bmQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n8R4n-00A0Y4-Gs; Fri, 14 Jan 2022 18:13:25 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n8R4j-00A0Xk-Kk for linux-arm-kernel@lists.infradead.org; Fri, 14 Jan 2022 18:13:23 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 614AEED1; Fri, 14 Jan 2022 10:13:20 -0800 (PST) Received: from donnerap.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A75203F766; Fri, 14 Jan 2022 10:13:19 -0800 (PST) Date: Fri, 14 Jan 2022 18:13:17 +0000 From: Andre Przywara To: Mark Rutland Cc: linux-arm-kernel@lists.infradead.org, Jaxson.Han@arm.com, Wei.Chen@arm.com Subject: Re: [bootwrapper PATCH 02/13] Add bit-field macros Message-ID: <20220114181317.5742b91a@donnerap.cambridge.arm.com> In-Reply-To: References: <20220111130653.2331827-1-mark.rutland@arm.com> <20220111130653.2331827-3-mark.rutland@arm.com> <20220111144048.2675076e@donnerap.cambridge.arm.com> Organization: ARM X-Mailer: Claws Mail 3.17.5 (GTK+ 2.24.32; aarch64-unknown-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220114_101321_813727_9458EC85 X-CRM114-Status: GOOD ( 38.48 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, 12 Jan 2022 14:16:21 +0000 Mark Rutland 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 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 > > > --- > > > 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