Re: [PATCH V2 18/46] arm64/sysreg: Add register fields for PMUACR_EL1

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

On 12/17/24 04:45, Rob Herring wrote:
> On Tue, Dec 10, 2024 at 11:22:43AM +0530, Anshuman Khandual wrote:
>> This adds register fields for PMUACR_EL1 as per the definitions based
>> on DDI0601 2024-09.
>>
>> 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/sysreg | 37 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
>> index 214ad6da1dff..462adb8031ca 100644
>> --- a/arch/arm64/tools/sysreg
>> +++ b/arch/arm64/tools/sysreg
>> @@ -2349,6 +2349,43 @@ Res0	63:5
>>  Field	4:0	SEL
>>  EndSysreg
>>  
>> +Sysreg	PMUACR_EL1	3	0	9	14	4
> 
> I already added this and various other PMUv3.9 registers you've added 
> here in v6.12 and v6.13. So are you on an old base or the tool allows 
> multiple definitions? If the latter, that should be fixed.

This series is based on v6.13-rc1 and as you mentioned PMUACR_EL1 has
already been added into tools sysreg.

Sysreg  PMUACR_EL1      3       0       9       14      4
Res0    63:33
Field   32      F0
Field   31      C
Field   30:0    P
EndSysreg

Seems like the tool does allow multiple definitions for a single register.
The generated header (arch/arm64/include/generated/asm/sysreg-defs.h) does
include redundant blocks for the following.

#define REG_PMUACR_EL1                                  S3_0_C9_C14_4
#define SYS_PMUACR_EL1                                  sys_reg(3, 0, 9, 14, 4)
#define SYS_PMUACR_EL1_Op0                              3
#define SYS_PMUACR_EL1_Op1                              0
#define SYS_PMUACR_EL1_CRn                              9
#define SYS_PMUACR_EL1_CRm                              14
#define SYS_PMUACR_EL1_Op2                              4

#define PMUACR_EL1_C                                    GENMASK(31, 31)
#define PMUACR_EL1_C_MASK                               GENMASK(31, 31)
#define PMUACR_EL1_C_SHIFT                              31
#define PMUACR_EL1_C_WIDTH                              1

I am wondering how this did not cause any re-definition warning ?

> 
>> +Res0	63:33
>> +Field	32	FM
>> +Field	31	C
>> +Field	30	P30
>> +Field	29	P29
>> +Field	28	P28
>> +Field	27	P27
>> +Field	26	P26
>> +Field	25	P25
>> +Field	24	P24
>> +Field	23	P23
>> +Field	22	P22
>> +Field	21	P21
>> +Field	20	P20
>> +Field	19	P19
>> +Field	18	P18
>> +Field	17	P17
>> +Field	16	P16
>> +Field	15	P15
>> +Field	14	P14
>> +Field	13	P13
>> +Field	12	P12
>> +Field	11	P11
>> +Field	10	P10
>> +Field	9	P9
>> +Field	8	P8
>> +Field	7	P7
>> +Field	6	P6
>> +Field	5	P5
>> +Field	4	P4
>> +Field	3	P3
>> +Field	2	P2
>> +Field	1	P1
>> +Field	0	P0
> 
> We're never going to use Pnn defines. This is just useless bloat unless 
> we're aiming to top amd gpu defines LOC.

Okay, this patch was trying to be cautiously comprehensive. But anyways
PMUACR_EL1 has already been added and hence this is redundant now.