Re: [PATCH 3/6] arm64/sysreg: Allow enumerations to be declared as signed

From: Mark Rutland <mark.rutland@arm.com>
To: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	James Morse <james.morse@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 3/6] arm64/sysreg: Allow enumerations to be declared as signed
Date: Fri, 9 Dec 2022 12:34:28 +0000	[thread overview]
Message-ID: <Y5MrVC3d8MPhvshE@FVFF77S0Q05N> (raw)
In-Reply-To: <20221207-arm64-sysreg-helpers-v1-3-149fa1308a23@kernel.org>

On Thu, Dec 08, 2022 at 04:03:24PM +0000, Mark Brown wrote:
> Most of our enumerations follow a standard scheme where the values can be
> treated as signed however there are some where the value must be treated
> as signed. Provide a new SignedEnum which allows this to be specified in
> the sysreg definition and emit a REG_FIELD_SIGN define for all enumerations
> which is a boolean corresponding to our current FTR_UNSIGNED and FTR_SIGNED
> macros.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

In future we might want to be explicit about which enums are unsigned feature
enums, and which are "just" enums without any ordering of values or sign (e.g.
CTR_EL0.L1Ip). i.e. we might want to explciitly add an `UnsignedEnum`, and have
plain `Enum` not define a <reg>_<field>_SIGN definition.

Minor nit: since <reg>_<field>_SIGN is a boolean, it'd make more sense for that
to be named <reg>_<field>_SIGNED.

Regardless, this makes sense to me, so even for this as-is:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/tools/gen-sysreg.awk | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/arm64/tools/gen-sysreg.awk b/arch/arm64/tools/gen-sysreg.awk
> index c350164a3955..e105d6fc66a4 100755
> --- a/arch/arm64/tools/gen-sysreg.awk
> +++ b/arch/arm64/tools/gen-sysreg.awk
> @@ -44,6 +44,11 @@ function define_field(reg, field, msb, lsb) {
>  	define(reg "_" field "_WIDTH", msb - lsb + 1)
>  }
>  
> +# Print a field _SIGNED definition for a field
> +function define_field_sign(reg, field, sign) {
> +	define(reg "_" field "_SIGN", sign)
> +}
> +
>  # Parse a "<msb>[:<lsb>]" string into the global variables @msb and @lsb
>  function parse_bitdef(reg, field, bitdef, _bits)
>  {
> @@ -233,6 +238,18 @@ END {
>  	next
>  }
>  
> +/^SignedEnum/ {
> +	change_block("Enum<", "Sysreg", "Enum")
> +	expect_fields(3)
> +	field = $3
> +	parse_bitdef(reg, field, $2)
> +
> +	define_field(reg, field, msb, lsb)
> +	define_field_sign(reg, field, "true")
> +
> +	next
> +}
> +
>  /^Enum/ {
>  	change_block("Enum", "Sysreg", "Enum")
>  	expect_fields(3)
> @@ -240,6 +257,7 @@ END {
>  	parse_bitdef(reg, field, $2)
>  
>  	define_field(reg, field, msb, lsb)
> +	define_field_sign(reg, field, "false")
>  
>  	next
>  }
> 
> -- 
> 2.30.2
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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