Re: [PATCH v5 00/12] arm64: Automatic system register definition generation

From: Mark Rutland <mark.rutland@arm.com>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Brown <broonie@kernel.org>, Will Deacon <will@kernel.org>,
	Marc Zyngier <maz@kernel.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 00/12] arm64: Automatic system register definition generation
Date: Tue, 3 May 2022 13:58:09 +0100	[thread overview]
Message-ID: <YnEm4cUqf39pFl3c@lakrids> (raw)
In-Reply-To: <YnER8hY/UG9MHs3X@arm.com>

On Tue, May 03, 2022 at 12:28:50PM +0100, Catalin Marinas wrote:
> On Tue, Apr 26, 2022 at 07:16:52PM +0100, Mark Brown wrote:
> > This patch series introduces a
> > script which describes registers and the fields within them in a format
> > that is easy to cross reference with the architecture reference manual
> > and uses them to generate the constants we use in a standard format:
> > 
> > | #define REG_ID_AA64ISAR0_EL1                    S3_0_C0_C6_0
> > | #define SYS_ID_AA64ISAR0_EL1                    sys_reg(3, 0, 0, 6, 0)
> > | #define SYS_ID_AA64ISAR0_EL1_Op0                3
> > | #define SYS_ID_AA64ISAR0_EL1_Op1                0
> > | #define SYS_ID_AA64ISAR0_EL1_CRn                0
> > | #define SYS_ID_AA64ISAR0_EL1_CRm                6
> > | #define SYS_ID_AA64ISAR0_EL1_Op2                0
> > 
> > | #define ID_AA64ISAR0_EL1_RNDR                   GENMASK(63, 60)
> > | #define ID_AA64ISAR0_EL1_RNDR_MASK              GENMASK(63, 60)
> > | #define ID_AA64ISAR0_EL1_RNDR_SHIFT             60
> > | #define ID_AA64ISAR0_EL1_RNDR_WIDTH             4
> > | #define ID_AA64ISAR0_EL1_RNDR_NI                UL(0b0000)
> > | #define ID_AA64ISAR0_EL1_RNDR_IMP               UL(0b0001)
> 
> I like the automatic generation of the register fields, I think it's
> less prone to errors and it gives us consistent notation, so it would be
> nice to have this in mainline.
> 
> I'm not convinced that we need the RES[01]_msb_lsb definitions, I'd
> rather have a RES[01] with all the UL(x) or'ed in.

FWIW, the scripting does define the combined RES[01] definitions with
all constituent bits or'd in. It just happens to define those in terms
of the individual RES[01]_msb_lsb definitions.

I had originally added the RES[01]_msb_lsb definitions purely so I could
reuse the define_field() logic, and I agree that we don't really need
the constituent portions to be defined individually (but I'm happy
either way).

I believe it's easy to remove the individual field definitions by having
the Res0 and Res1 block handlers not call define_field(), and add add to
the res[01] vars directly, e.g.

| /^Res1/ && (block == "Sysreg" || block == "SysregFields") {
|        expect_fields(2)
|        parse_bitdef(reg, "RES1", $2)
|
|        res1 = res1 " | GENMASK(" msb ", " lsb ")"
|
|        next
| }

> > Rather than attempting to convert every register at once the current
> > series converts a few sample registers to provide some concrete examples
> > but allow for easier updating during review of the file format and the
> > script.  Handling a register at a time should also make review less
> > taxing so it seems like a sensible approach in general.
> 
> So apart from RES definitions (but happy to hear an argument), I'm fine
> with the patches. If you think they are ready, we could queue the
> infrastructure for 5.19 and add some of the register definitions at -rc1
> to avoid conflicts (the more conversions the better).

FWIW, that sounds good to me!

Thanks,
Mark.

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