Re: [PATCH 1/1] arm64/tools/sysreg: Add Sysreg128/SysregFields128

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 8/3/24 16:42, Mark Rutland wrote:
> On Thu, Aug 01, 2024 at 11:14:36AM +0530, Anshuman Khandual wrote:
>> FEAT_SYSREG128 enables 128 bit wide system registers which also need to be
>> defined in (arch/arm64/toos/sysreg) for auto mask generation. This adds two
>> new field types i.e Sysreg128 and SysregFields128 for that same purpose. It
>> utilizes recently added macro GENMASK_U128() while also adding some helpers
>> such as define_field_128() and parse_bitdef_128().
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/tools/gen-sysreg.awk | 231 ++++++++++++++++++++++++++++++++
>>  1 file changed, 231 insertions(+)
> Having Sysreg128 and SysregFields128 sounds reasonable, though I suspect
> we can share more code with Sysreg and SysregFields (e.g. by always
> using GENMASK128() even for regular SYsreg and SysregFIilds).

GENMASK_U128() should deliver same bit masks that could be used instead
of earlier GENMASK_ULL() bit masks. Sounds good.

> 
> Regardless of this patch in particular, I think we want to see some
> end-to-end usage (i.e. some actual bit definitions, along with asm and C
> code that uses these definitions) so that we're confident this is the
> right way to capture these definitions.
> 
> Sending this piecemeal, separate to those elements and sepate to
> GENMASK_U128() makes this very painful to review effectively. Please
> combine those elements into a single series so that reviewers can see
> the entire picture.

Sure, will try and combine the elements as as much possible.