On 9/29/22 16:59, Mark Brown wrote:

On Thu, Sep 29, 2022 at 01:28:51PM +0530, Anshuman Khandual wrote:

Thanks for doing this work - I did spot a few small issues though.

quoted

 /* id_aa64dfr0 */
+#define ID_AA64DFR0_BRBE_SHIFT		52
 #define ID_AA64DFR0_MTPMU_SHIFT		48
 #define ID_AA64DFR0_TRBE_SHIFT		44
 #define ID_AA64DFR0_TRACE_FILT_SHIFT	40

@@ -848,6 +952,9 @@
 #define ID_AA64DFR0_PMSVER_8_2		0x1
 #define ID_AA64DFR0_PMSVER_8_3		0x2
 
+#define ID_AA64DFR0_BRBE		0x1
+#define ID_AA64DFR0_BRBE_V1P1		0x2
+
 #define ID_DFR0_PERFMON_SHIFT		24
 
 #define ID_DFR0_PERFMON_8_0		0x3

This is already done in -next as a result of ID_AA64DFR0_EL1 being
converted, the enumberation define comes out as
ID_AA64DFR0_EL1_BRBE_BRBE_V1P1.

Right. I will rebase the series on upcoming 6.1-rc1 which should have all the
current -next patches including ID_AA64DFR0_EL1 migration into tools/sysreg.
This should just fall in place.

quoted

+# This is just a dummy register declaration to get all common field masks and
+# shifts for accessing given BRBINF contents.
+Sysreg	BRBINF_EL1	2	1	8	0	0
+Res0	63:47
+Field	46	CCU
+Field	45:32	CC
+Res0	31:18
+Field	17	LASTFAILED
+Field	16	TX

According to DDI0487I.a bit 16 is called T not TX.

I understand :) The intention here was to keep the field name associated with
"transaction" some how. But I guess, it would be more important to keep it as
is matching the ARM ARM than something for readability purpose. Will change it
as 'T'.

quoted

+Res0	15:14
+Enum	13:8		TYPE

It's probably worth noting in the comment the issue with Enums here
that's meaning you're not using a SysregFields - I'm not sure what

Sure, will add a comment describing the problem of using enum elements inside
SysregFields definition.

people will think of this but providing a definition using the ID for
the 0th register does seem expedient.

I understand your concern but this turned out to be a better option

- Original sysreg.h based definitions had all field masks on the right end

	- When reading (reg >> field_shift) & field_mask
	- When writing (val & field_mask) << field_shift

- tools/sysreg format creates in-place field masks

	- When reading (reg & field_mask) >> field_shift
	- When writing (val << field_shift) & field_mask

- After moving some BRBE registers into tools/sysreg, the driver code had
  to be changed to accommodate these new write/read methods

- To avoid mix up in the BRBE driver, all BRBINF register fields need to be
  converted into in place masks, either in sysreg.h itself or moving into
  tools/sysreg

Moving BRBE fields into tools/sysreg via a dummy BRBINF_EL1 register seems
to achieve that objective. These common fields can be used to work on any
BRBINF<N>_EL1 register value. But I might just keep them in sysreg.h, if
the proposed solution is not preferable or seems expedient.

Later when enum support comes up in SysregFields and tools/sysreg supports
formula based crm/op2 expansion entire BRBINF, BRBSRC, BRBTGT register set
can be moved into tools/sysreg.

quoted

+Enum	7:6	EL
+	0b00	EL0
+	0b01	EL1
+	0b10	EL2
+EndEnum

According to DDI0487I.a 0b11 has the value EL3 (when FEAT_BRBEv1p1).

Sure, will add it.

quoted

+Sysreg	BRBCR_EL1	2	1	9	0	0
+Res0	63:24
+Field	23 	EXCEPTION
+Field	22 	ERTN
+Res0	21:9
+Field	8 	FZP
+Field	7	ZERO

According to DDI0487I.a bit 7 is Res0.

Sure, will change.

quoted

+Field	2	ZERO1

According to DDI0487I.a bit 2 is Res0.

Sure, will change.

quoted

+Sysreg	BRBFCR_EL1	2	1	9	0	1

quoted

+Field	16	ENL

Accoding to DDI0487I.a this is EnI (ie, an L not an I).

ENL != Enl ? Do we need to match the case as well ?

quoted

+Sysreg	BRBINFINJ_EL1	2	1	9	1	0

quoted

+Field	16	TX

According to DDI0487I.a this is T not TX.

As mentioned, will change it as 'T'.

quoted

+Enum	7:6	EL
+	0b00	EL0
+	0b01	EL1
+	0b10	EL2
+EndEnum

According to DDI0487I.a 0b11 has the value EL3 (when FEAT_BRBEv1p1).

Sure, will add.

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