[PATCH v2 00/15] Make ETM register accesses consistent with sysreg.h

[PATCH v2 00/15] Make ETM register accesses consistent with sysreg.h

[James Clark]

Changes since v1:
  * Change base to "[PATCH 1/1] coresight: no-op refactor to make INSTP0 check more idiomatic"
  * Change 'name' to 'field' in REG_VAL() macro
  * Add comment for REG_VAL() macro

While working on the branch broadcast change I found it difficult to search
for usages of registers and fields because of the magic numbers. I also
found it difficult to decide which style to make new code in because of the
varying ones used.

There was also a code review comment from Suzuki about replacing a magic
number so I'm proposing to refactor as many as possible into the style used
in sysreg.h which seems to be the new and most consistently used method.
For example it was used in the SPE and TRBE drivers.

This isn't an exhaustive refactor, but it does get all the basic accesses.
There are a couple of odd other cases remaining, mainly in the ETM3x code.
These can be found by searching for BMVAL.

There is one compromise to ensure this is a complete no-op and has
binary equivalence with the old version. I needed to keep two register
accesses here, where something like etmidr0 & TRCIDR0_INSTP0_LOAD_STORE
would be better:

  -	if (BMVAL(etmidr0, 1, 1) && BMVAL(etmidr0, 2, 2))
  -		drvdata->instrp0 = true;
  -	else
  -		drvdata->instrp0 = false;
  -
  +	drvdata->instrp0 = !!((REG_VAL(etmidr0, TRCIDR0_INSTP0) & 0b01) &&
  +			      (REG_VAL(etmidr0, TRCIDR0_INSTP0) & 0b10));

I think this change fixes quite a few issues like:

 * Some registers aren't referred to by name but a different variable name.
   For example eventctrl1 in mode_store() where TRCEVENTCTL1R isn't
   mentioned in that function.

 * Some bits aren't referred to by the name in the manual, even in the
   comments. For example TRCCONFIGR.RS only occurs as /* bit[12], Return
   stack enable bit */.

 * Some bits in the same register are referred to either as magic numbers
   or the publicly exported config macros, neiher of which are consistent
   with any other register accesses. For example

   config->cfg |= BIT(11);
   config->cfg |= BIT(ETM4_CFG_BIT_CTXTID);
   
 * Some fields are already partially referred to in the sysfs.h style way:
   TRCVICTLR_EXLEVEL_... etc. But other fields in the same register are not
   
 * Some fields are magic numbers that are repeated many times in different
   functions. For example vinst_ctrl |= BIT(9)

 * Some fields were referred to by magic numbers, even when there were
   already existing #defines. For example ETMTECR1_INC_EXC

 * Another benefit is that the #defines could be automatically checked
   against the reference manual because the style is uniform.

Testing
=======

To test this I used gcc-11 which doesn't have a quirk about changing
register widths in some cases (as in w -> x). I also used elf_diff which
showed me exactly where I'd made a mistake when I did
(https://github.com/noseglasses/elf_diff). But now that there is no
difference, objdump and diff also work for validation.

  make CC=gcc-11 modules
  diff <(objdump -d drivers/hwtracing/coresight/coresight-etm4x.ko) <(objdump -d ../linux2/drivers/hwtracing/coresight/coresight-etm4x.ko)
  diff <(objdump -d drivers/hwtracing/coresight/coresight.ko) <(objdump -d ../linux2/drivers/hwtracing/coresight/coresight.ko)

And for ETM3x (doesn't need gcc 11):

  make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf-  modules
  diff <(objdump -d drivers/hwtracing/coresight/coresight-etm3x.ko) <(objdump -d ../linux2/drivers/hwtracing/coresight/coresight-etm3x.ko)

When there are no differences, the diff output looks like this with only
the filename listed:

  < drivers/hwtracing/coresight/coresight-etm4x.ko:     file format elf64-littleaarch64
  ---
  > ../linux2/drivers/hwtracing/coresight/coresight-etm4x.ko:     file format elf64-littleaarch64

Applies to "[PATCH 1/1] coresight: no-op refactor to make INSTP0 check more idiomatic"

James Clark (15):
  coresight: Make ETM4x TRCIDR0 register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCIDR2 register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCIDR3 register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCIDR4 register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCIDR5 register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCCONFIGR register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCEVENTCTL1R register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCSTALLCTLR register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCVICTLR register accesses consistent with
    sysreg.h
  coresight: Make ETM3x ETMTECR1 register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCACATRn register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCSSCCRn and TRCSSCSRn register accesses
    consistent with sysreg.h
  coresight: Make ETM4x TRCSSPCICRn register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCBBCTLR register accesses consistent with
    sysreg.h
  coresight: Make ETM4x TRCRSCTLRn register accesses consistent with
    sysreg.h

 .../coresight/coresight-etm3x-core.c          |   2 +-
 .../coresight/coresight-etm3x-sysfs.c         |   2 +-
 .../coresight/coresight-etm4x-core.c          | 134 +++++--------
 .../coresight/coresight-etm4x-sysfs.c         | 178 +++++++++---------
 drivers/hwtracing/coresight/coresight-etm4x.h | 159 ++++++++++++++--
 drivers/hwtracing/coresight/coresight-priv.h  |   5 +
 6 files changed, 286 insertions(+), 194 deletions(-)

-- 
2.28.0

[PATCH v2 01/15] coresight: Make ETM4x TRCIDR0 register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 .../coresight/coresight-etm4x-core.c          | 36 +++++--------------
 drivers/hwtracing/coresight/coresight-etm4x.h | 17 +++++++++
 drivers/hwtracing/coresight/coresight-priv.h  |  5 +++
 3 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index e2eebd865241..107e81948f76 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1091,41 +1091,21 @@ static void etm4_init_arch_data(void *info)
 	etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
 
 	/* INSTP0, bits[2:1] P0 tracing support field */
-	if (BMVAL(etmidr0, 1, 2) == 0b11)
-		drvdata->instrp0 = true;
-	else
-		drvdata->instrp0 = false;
-
+	drvdata->instrp0 = !!(REG_VAL(etmidr0, TRCIDR0_INSTP0) == 0b11);
 	/* TRCBB, bit[5] Branch broadcast tracing support bit */
-	if (BMVAL(etmidr0, 5, 5))
-		drvdata->trcbb = true;
-	else
-		drvdata->trcbb = false;
-
+	drvdata->trcbb = !!(etmidr0 & TRCIDR0_TRCBB);
 	/* TRCCOND, bit[6] Conditional instruction tracing support bit */
-	if (BMVAL(etmidr0, 6, 6))
-		drvdata->trccond = true;
-	else
-		drvdata->trccond = false;
-
+	drvdata->trccond = !!(etmidr0 & TRCIDR0_TRCCOND);
 	/* TRCCCI, bit[7] Cycle counting instruction bit */
-	if (BMVAL(etmidr0, 7, 7))
-		drvdata->trccci = true;
-	else
-		drvdata->trccci = false;
-
+	drvdata->trccci = !!(etmidr0 & TRCIDR0_TRCCCI);
 	/* RETSTACK, bit[9] Return stack bit */
-	if (BMVAL(etmidr0, 9, 9))
-		drvdata->retstack = true;
-	else
-		drvdata->retstack = false;
-
+	drvdata->retstack = !!(etmidr0 & TRCIDR0_RETSTACK);
 	/* NUMEVENT, bits[11:10] Number of events field */
-	drvdata->nr_event = BMVAL(etmidr0, 10, 11);
+	drvdata->nr_event = REG_VAL(etmidr0, TRCIDR0_NUMEVENT);
 	/* QSUPP, bits[16:15] Q element support field */
-	drvdata->q_support = BMVAL(etmidr0, 15, 16);
+	drvdata->q_support = REG_VAL(etmidr0, TRCIDR0_QSUPP);
 	/* TSSIZE, bits[28:24] Global timestamp size field */
-	drvdata->ts_size = BMVAL(etmidr0, 24, 28);
+	drvdata->ts_size = REG_VAL(etmidr0, TRCIDR0_TSSIZE);
 
 	/* maximum size of resources */
 	etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 3c4d69b096ca..2bd8ad953b8e 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -130,6 +130,23 @@
 
 #define TRCRSR_TA			BIT(12)
 
+/*
+ * Bit positions of registers that are defined above, in the sysreg.h style
+ * of _MASK, _SHIFT and BIT().
+ */
+#define TRCIDR0_INSTP0_SHIFT			1
+#define TRCIDR0_INSTP0_MASK			GENMASK(1, 0)
+#define TRCIDR0_TRCBB				BIT(5)
+#define TRCIDR0_TRCCOND				BIT(6)
+#define TRCIDR0_TRCCCI				BIT(7)
+#define TRCIDR0_RETSTACK			BIT(9)
+#define TRCIDR0_NUMEVENT_SHIFT			10
+#define TRCIDR0_NUMEVENT_MASK			GENMASK(1, 0)
+#define TRCIDR0_QSUPP_SHIFT			15
+#define TRCIDR0_QSUPP_MASK			GENMASK(1, 0)
+#define TRCIDR0_TSSIZE_SHIFT			24
+#define TRCIDR0_TSSIZE_MASK			GENMASK(4, 0)
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index ff1dd2092ac5..1452c6038421 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -36,6 +36,11 @@
 
 #define TIMEOUT_US		100
 #define BMVAL(val, lsb, msb)	((val & GENMASK(msb, lsb)) >> lsb)
+/*
+ * Extract a field from a register where field is #defined in the form
+ * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
+ */
+#define REG_VAL(val, field)	((val & (field##_MASK << field##_SHIFT)) >> field##_SHIFT)
 
 #define ETM_MODE_EXCL_KERN	BIT(30)
 #define ETM_MODE_EXCL_USER	BIT(31)
-- 
2.28.0

[PATCH v2 02/15] coresight: Make ETM4x TRCIDR2 register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 6 +++---
 drivers/hwtracing/coresight/coresight-etm4x.h      | 7 +++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 107e81948f76..891cfcd93f94 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1110,11 +1110,11 @@ static void etm4_init_arch_data(void *info)
 	/* maximum size of resources */
 	etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
 	/* CIDSIZE, bits[9:5] Indicates the Context ID size */
-	drvdata->ctxid_size = BMVAL(etmidr2, 5, 9);
+	drvdata->ctxid_size = REG_VAL(etmidr2, TRCIDR2_CIDSIZE);
 	/* VMIDSIZE, bits[14:10] Indicates the VMID size */
-	drvdata->vmid_size = BMVAL(etmidr2, 10, 14);
+	drvdata->vmid_size = REG_VAL(etmidr2, TRCIDR2_VMIDSIZE);
 	/* CCSIZE, bits[28:25] size of the cycle counter in bits minus 12 */
-	drvdata->ccsize = BMVAL(etmidr2, 25, 28);
+	drvdata->ccsize = REG_VAL(etmidr2, TRCIDR2_CCSIZE);
 
 	etmidr3 = etm4x_relaxed_read32(csa, TRCIDR3);
 	/* CCITMIN, bits[11:0] minimum threshold value that can be programmed */
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 2bd8ad953b8e..a95df5686b4b 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -147,6 +147,13 @@
 #define TRCIDR0_TSSIZE_SHIFT			24
 #define TRCIDR0_TSSIZE_MASK			GENMASK(4, 0)
 
+#define TRCIDR2_CIDSIZE_SHIFT			5
+#define TRCIDR2_CIDSIZE_MASK			GENMASK(4, 0)
+#define TRCIDR2_VMIDSIZE_SHIFT			10
+#define TRCIDR2_VMIDSIZE_MASK			GENMASK(4, 0)
+#define TRCIDR2_CCSIZE_SHIFT			25
+#define TRCIDR2_CCSIZE_MASK			GENMASK(3, 0)
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
-- 
2.28.0

[PATCH v2 03/15] coresight: Make ETM4x TRCIDR3 register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 .../coresight/coresight-etm4x-core.c          | 40 +++++--------------
 drivers/hwtracing/coresight/coresight-etm4x.h | 15 +++++++
 2 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 891cfcd93f94..ba43fb9a4526 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1118,53 +1118,33 @@ static void etm4_init_arch_data(void *info)
 
 	etmidr3 = etm4x_relaxed_read32(csa, TRCIDR3);
 	/* CCITMIN, bits[11:0] minimum threshold value that can be programmed */
-	drvdata->ccitmin = BMVAL(etmidr3, 0, 11);
+	drvdata->ccitmin = REG_VAL(etmidr3, TRCIDR3_CCITMIN);
 	/* EXLEVEL_S, bits[19:16] Secure state instruction tracing */
-	drvdata->s_ex_level = BMVAL(etmidr3, 16, 19);
+	drvdata->s_ex_level = REG_VAL(etmidr3, TRCIDR3_EXLEVEL_S);
 	drvdata->config.s_ex_level = drvdata->s_ex_level;
 	/* EXLEVEL_NS, bits[23:20] Non-secure state instruction tracing */
-	drvdata->ns_ex_level = BMVAL(etmidr3, 20, 23);
-
+	drvdata->ns_ex_level = REG_VAL(etmidr3, TRCIDR3_EXLEVEL_NS);
 	/*
 	 * TRCERR, bit[24] whether a trace unit can trace a
 	 * system error exception.
 	 */
-	if (BMVAL(etmidr3, 24, 24))
-		drvdata->trc_error = true;
-	else
-		drvdata->trc_error = false;
-
+	drvdata->trc_error = !!(etmidr3 & TRCIDR3_TRCERR);
 	/* SYNCPR, bit[25] implementation has a fixed synchronization period? */
-	if (BMVAL(etmidr3, 25, 25))
-		drvdata->syncpr = true;
-	else
-		drvdata->syncpr = false;
-
+	drvdata->syncpr = !!(etmidr3 & TRCIDR3_SYNCPR);
 	/* STALLCTL, bit[26] is stall control implemented? */
-	if (BMVAL(etmidr3, 26, 26))
-		drvdata->stallctl = true;
-	else
-		drvdata->stallctl = false;
-
+	drvdata->stallctl = !!(etmidr3 & TRCIDR3_STALLCTL);
 	/* SYSSTALL, bit[27] implementation can support stall control? */
-	if (BMVAL(etmidr3, 27, 27))
-		drvdata->sysstall = true;
-	else
-		drvdata->sysstall = false;
-
+	drvdata->sysstall = !!(etmidr3 & TRCIDR3_SYSSTALL);
 	/*
 	 * NUMPROC - the number of PEs available for tracing, 5bits
 	 *         = TRCIDR3.bits[13:12]bits[30:28]
 	 *  bits[4:3] = TRCIDR3.bits[13:12] (since etm-v4.2, otherwise RES0)
 	 *  bits[3:0] = TRCIDR3.bits[30:28]
 	 */
-	drvdata->nr_pe = (BMVAL(etmidr3, 12, 13) << 3) | BMVAL(etmidr3, 28, 30);
-
+	drvdata->nr_pe = (REG_VAL(etmidr3, TRCIDR3_NUMPROC_HI) << 3) |
+			  REG_VAL(etmidr3, TRCIDR3_NUMPROC_LO);
 	/* NOOVERFLOW, bit[31] is trace overflow prevention supported */
-	if (BMVAL(etmidr3, 31, 31))
-		drvdata->nooverflow = true;
-	else
-		drvdata->nooverflow = false;
+	drvdata->nooverflow = !!(etmidr3 & TRCIDR3_NOOVERFLOW);
 
 	/* number of resources trace unit supports */
 	etmidr4 = etm4x_relaxed_read32(csa, TRCIDR4);
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index a95df5686b4b..051d7948f15b 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -154,6 +154,21 @@
 #define TRCIDR2_CCSIZE_SHIFT			25
 #define TRCIDR2_CCSIZE_MASK			GENMASK(3, 0)
 
+#define TRCIDR3_CCITMIN_SHIFT			0
+#define TRCIDR3_CCITMIN_MASK			GENMASK(11, 0)
+#define TRCIDR3_EXLEVEL_S_SHIFT			16
+#define TRCIDR3_EXLEVEL_S_MASK			GENMASK(3, 0)
+#define TRCIDR3_EXLEVEL_NS_SHIFT		20
+#define TRCIDR3_EXLEVEL_NS_MASK			GENMASK(3, 0)
+#define TRCIDR3_TRCERR				BIT(24)
+#define TRCIDR3_SYNCPR				BIT(25)
+#define TRCIDR3_STALLCTL			BIT(26)
+#define TRCIDR3_SYSSTALL			BIT(27)
+#define TRCIDR3_NUMPROC_LO_SHIFT		28
+#define TRCIDR3_NUMPROC_LO_MASK			GENMASK(2, 0)
+#define TRCIDR3_NUMPROC_HI_SHIFT		12
+#define TRCIDR3_NUMPROC_HI_MASK			GENMASK(1, 0)
+#define TRCIDR3_NOOVERFLOW			BIT(31)
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
-- 
2.28.0

[PATCH v2 04/15] coresight: Make ETM4x TRCIDR4 register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 12 ++++++------
 drivers/hwtracing/coresight/coresight-etm4x.h      | 14 ++++++++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index ba43fb9a4526..553a532b65f8 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1149,9 +1149,9 @@ static void etm4_init_arch_data(void *info)
 	/* number of resources trace unit supports */
 	etmidr4 = etm4x_relaxed_read32(csa, TRCIDR4);
 	/* NUMACPAIRS, bits[0:3] number of addr comparator pairs for tracing */
-	drvdata->nr_addr_cmp = BMVAL(etmidr4, 0, 3);
+	drvdata->nr_addr_cmp = REG_VAL(etmidr4, TRCIDR4_NUMACPAIRS);
 	/* NUMPC, bits[15:12] number of PE comparator inputs for tracing */
-	drvdata->nr_pe_cmp = BMVAL(etmidr4, 12, 15);
+	drvdata->nr_pe_cmp = REG_VAL(etmidr4, TRCIDR4_NUMPC);
 	/*
 	 * NUMRSPAIR, bits[19:16]
 	 * The number of resource pairs conveyed by the HW starts at 0, i.e a
@@ -1162,7 +1162,7 @@ static void etm4_init_arch_data(void *info)
 	 * the default TRUE and FALSE resource selectors are omitted.
 	 * Otherwise for values 0x1 and above the number is N + 1 as per v4.2.
 	 */
-	drvdata->nr_resource = BMVAL(etmidr4, 16, 19);
+	drvdata->nr_resource = REG_VAL(etmidr4, TRCIDR4_NUMRSPAIR);
 	if ((drvdata->arch < ETM_ARCH_V4_3) || (drvdata->nr_resource > 0))
 		drvdata->nr_resource += 1;
 	/*
@@ -1170,15 +1170,15 @@ static void etm4_init_arch_data(void *info)
 	 * comparator control for tracing. Read any status regs as these
 	 * also contain RO capability data.
 	 */
-	drvdata->nr_ss_cmp = BMVAL(etmidr4, 20, 23);
+	drvdata->nr_ss_cmp = REG_VAL(etmidr4, TRCIDR4_NUMSSCC);
 	for (i = 0; i < drvdata->nr_ss_cmp; i++) {
 		drvdata->config.ss_status[i] =
 			etm4x_relaxed_read32(csa, TRCSSCSRn(i));
 	}
 	/* NUMCIDC, bits[27:24] number of Context ID comparators for tracing */
-	drvdata->numcidc = BMVAL(etmidr4, 24, 27);
+	drvdata->numcidc = REG_VAL(etmidr4, TRCIDR4_NUMCIDC);
 	/* NUMVMIDC, bits[31:28] number of VMID comparators for tracing */
-	drvdata->numvmidc = BMVAL(etmidr4, 28, 31);
+	drvdata->numvmidc = REG_VAL(etmidr4, TRCIDR4_NUMVMIDC);
 
 	etmidr5 = etm4x_relaxed_read32(csa, TRCIDR5);
 	/* NUMEXTIN, bits[8:0] number of external inputs implemented */
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 051d7948f15b..0b22c57a9da1 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -169,6 +169,20 @@
 #define TRCIDR3_NUMPROC_HI_SHIFT		12
 #define TRCIDR3_NUMPROC_HI_MASK			GENMASK(1, 0)
 #define TRCIDR3_NOOVERFLOW			BIT(31)
+
+#define TRCIDR4_NUMACPAIRS_SHIFT		0
+#define TRCIDR4_NUMACPAIRS_MASK			GENMASK(3, 0)
+#define TRCIDR4_NUMPC_SHIFT			12
+#define TRCIDR4_NUMPC_MASK			GENMASK(3, 0)
+#define TRCIDR4_NUMRSPAIR_SHIFT			16
+#define TRCIDR4_NUMRSPAIR_MASK			GENMASK(3, 0)
+#define TRCIDR4_NUMSSCC_SHIFT			20
+#define TRCIDR4_NUMSSCC_MASK			GENMASK(3, 0)
+#define TRCIDR4_NUMCIDC_SHIFT			24
+#define TRCIDR4_NUMCIDC_MASK			GENMASK(3, 0)
+#define TRCIDR4_NUMVMIDC_SHIFT			28
+#define TRCIDR4_NUMVMIDC_MASK			GENMASK(3, 0)
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
-- 
2.28.0

[PATCH v2 05/15] coresight: Make ETM4x TRCIDR5 register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 .../hwtracing/coresight/coresight-etm4x-core.c | 18 ++++++------------
 drivers/hwtracing/coresight/coresight-etm4x.h  | 11 +++++++++++
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 553a532b65f8..0f0628968bfd 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1182,26 +1182,20 @@ static void etm4_init_arch_data(void *info)
 
 	etmidr5 = etm4x_relaxed_read32(csa, TRCIDR5);
 	/* NUMEXTIN, bits[8:0] number of external inputs implemented */
-	drvdata->nr_ext_inp = BMVAL(etmidr5, 0, 8);
+	drvdata->nr_ext_inp = REG_VAL(etmidr5, TRCIDR5_NUMEXTIN);
 	/* TRACEIDSIZE, bits[21:16] indicates the trace ID width */
-	drvdata->trcid_size = BMVAL(etmidr5, 16, 21);
+	drvdata->trcid_size = REG_VAL(etmidr5, TRCIDR5_TRACEIDSIZE);
 	/* ATBTRIG, bit[22] implementation can support ATB triggers? */
-	if (BMVAL(etmidr5, 22, 22))
-		drvdata->atbtrig = true;
-	else
-		drvdata->atbtrig = false;
+	drvdata->atbtrig = !!(etmidr5 & TRCIDR5_ATBTRIG);
 	/*
 	 * LPOVERRIDE, bit[23] implementation supports
 	 * low-power state override
 	 */
-	if (BMVAL(etmidr5, 23, 23) && (!drvdata->skip_power_up))
-		drvdata->lpoverride = true;
-	else
-		drvdata->lpoverride = false;
+	drvdata->lpoverride = (etmidr5 & TRCIDR5_LPOVERRIDE) && (!drvdata->skip_power_up);
 	/* NUMSEQSTATE, bits[27:25] number of sequencer states implemented */
-	drvdata->nrseqstate = BMVAL(etmidr5, 25, 27);
+	drvdata->nrseqstate = REG_VAL(etmidr5, TRCIDR5_NUMSEQSTATE);
 	/* NUMCNTR, bits[30:28] number of counters available for tracing */
-	drvdata->nr_cntr = BMVAL(etmidr5, 28, 30);
+	drvdata->nr_cntr = REG_VAL(etmidr5, TRCIDR5_NUMCNTR);
 	etm4_cs_lock(drvdata, csa);
 	cpu_detect_trace_filtering(drvdata);
 }
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 0b22c57a9da1..ca6ed39ceaf7 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -183,6 +183,17 @@
 #define TRCIDR4_NUMVMIDC_SHIFT			28
 #define TRCIDR4_NUMVMIDC_MASK			GENMASK(3, 0)
 
+#define TRCIDR5_NUMEXTIN_SHIFT			0
+#define TRCIDR5_NUMEXTIN_MASK			GENMASK(8, 0)
+#define TRCIDR5_TRACEIDSIZE_SHIFT		16
+#define TRCIDR5_TRACEIDSIZE_MASK		GENMASK(5, 0)
+#define TRCIDR5_ATBTRIG				BIT(22)
+#define TRCIDR5_LPOVERRIDE			BIT(23)
+#define TRCIDR5_NUMSEQSTATE_SHIFT		25
+#define TRCIDR5_NUMSEQSTATE_MASK		GENMASK(2, 0)
+#define TRCIDR5_NUMCNTR_SHIFT			28
+#define TRCIDR5_NUMCNTR_MASK			GENMASK(2, 0)
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
-- 
2.28.0

[PATCH v2 06/15] coresight: Make ETM4x TRCCONFIGR register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 .../coresight/coresight-etm4x-core.c          | 10 ++--
 .../coresight/coresight-etm4x-sysfs.c         | 46 +++++++++----------
 drivers/hwtracing/coresight/coresight-etm4x.h | 17 +++++++
 3 files changed, 45 insertions(+), 28 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 0f0628968bfd..dc936fb1cc74 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -633,7 +633,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
 
 	/* Go from generic option to ETMv4 specifics */
 	if (attr->config & BIT(ETM_OPT_CYCACC)) {
-		config->cfg |= BIT(4);
+		config->cfg |= TRCCONFIGR_CCI;
 		/* TRM: Must program this for cycacc to work */
 		config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT;
 	}
@@ -653,12 +653,12 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
 			goto out;
 
 		/* bit[11], Global timestamp tracing bit */
-		config->cfg |= BIT(11);
+		config->cfg |= TRCCONFIGR_TS;
 	}
 
 	if (attr->config & BIT(ETM_OPT_CTXTID))
 		/* bit[6], Context ID tracing bit */
-		config->cfg |= BIT(ETM4_CFG_BIT_CTXTID);
+		config->cfg |= TRCCONFIGR_CID;
 
 	/*
 	 * If set bit ETM_OPT_CTXTID2 in perf config, this asks to trace VMID
@@ -670,13 +670,13 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
 			ret = -EINVAL;
 			goto out;
 		}
-		config->cfg |= BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT);
+		config->cfg |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT;
 	}
 
 	/* return stack - enable if selected and supported */
 	if ((attr->config & BIT(ETM_OPT_RETSTK)) && drvdata->retstack)
 		/* bit[12], Return stack enable bit */
-		config->cfg |= BIT(12);
+		config->cfg |= TRCCONFIGR_RS;
 
 	/*
 	 * Set any selected configuration and preset.
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index 57e94424a8d6..4c29ab4464a0 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -180,12 +180,12 @@ static ssize_t reset_store(struct device *dev,
 
 	/* Disable data tracing: do not trace load and store data transfers */
 	config->mode &= ~(ETM_MODE_LOAD | ETM_MODE_STORE);
-	config->cfg &= ~(BIT(1) | BIT(2));
+	config->cfg &= ~(TRCCONFIGR_INSTP0_LOAD | TRCCONFIGR_INSTP0_STORE);
 
 	/* Disable data value and data address tracing */
 	config->mode &= ~(ETM_MODE_DATA_TRACE_ADDR |
 			   ETM_MODE_DATA_TRACE_VAL);
-	config->cfg &= ~(BIT(16) | BIT(17));
+	config->cfg &= ~(TRCCONFIGR_DA | TRCCONFIGR_DV);
 
 	/* Disable all events tracing */
 	config->eventctrl0 = 0x0;
@@ -304,82 +304,82 @@ static ssize_t mode_store(struct device *dev,
 
 	if (drvdata->instrp0 == true) {
 		/* start by clearing instruction P0 field */
-		config->cfg  &= ~(BIT(1) | BIT(2));
+		config->cfg  &= ~TRCCONFIGR_INSTP0_LOAD_STORE;
 		if (config->mode & ETM_MODE_LOAD)
 			/* 0b01 Trace load instructions as P0 instructions */
-			config->cfg  |= BIT(1);
+			config->cfg  |= TRCCONFIGR_INSTP0_LOAD;
 		if (config->mode & ETM_MODE_STORE)
 			/* 0b10 Trace store instructions as P0 instructions */
-			config->cfg  |= BIT(2);
+			config->cfg  |= TRCCONFIGR_INSTP0_STORE;
 		if (config->mode & ETM_MODE_LOAD_STORE)
 			/*
 			 * 0b11 Trace load and store instructions
 			 * as P0 instructions
 			 */
-			config->cfg  |= BIT(1) | BIT(2);
+			config->cfg  |= TRCCONFIGR_INSTP0_LOAD_STORE;
 	}
 
 	/* bit[3], Branch broadcast mode */
 	if ((config->mode & ETM_MODE_BB) && (drvdata->trcbb == true))
-		config->cfg |= BIT(3);
+		config->cfg |= TRCCONFIGR_BB;
 	else
-		config->cfg &= ~BIT(3);
+		config->cfg &= ~TRCCONFIGR_BB;
 
 	/* bit[4], Cycle counting instruction trace bit */
 	if ((config->mode & ETMv4_MODE_CYCACC) &&
 		(drvdata->trccci == true))
-		config->cfg |= BIT(4);
+		config->cfg |= TRCCONFIGR_CCI;
 	else
-		config->cfg &= ~BIT(4);
+		config->cfg &= ~TRCCONFIGR_CCI;
 
 	/* bit[6], Context ID tracing bit */
 	if ((config->mode & ETMv4_MODE_CTXID) && (drvdata->ctxid_size))
-		config->cfg |= BIT(6);
+		config->cfg |= TRCCONFIGR_CID;
 	else
-		config->cfg &= ~BIT(6);
+		config->cfg &= ~TRCCONFIGR_CID;
 
 	if ((config->mode & ETM_MODE_VMID) && (drvdata->vmid_size))
-		config->cfg |= BIT(7);
+		config->cfg |= TRCCONFIGR_VMID;
 	else
-		config->cfg &= ~BIT(7);
+		config->cfg &= ~TRCCONFIGR_VMID;
 
 	/* bits[10:8], Conditional instruction tracing bit */
 	mode = ETM_MODE_COND(config->mode);
 	if (drvdata->trccond == true) {
-		config->cfg &= ~(BIT(8) | BIT(9) | BIT(10));
-		config->cfg |= mode << 8;
+		config->cfg &= ~(TRCCONFIGR_COND_MASK << TRCCONFIGR_COND_SHIFT);
+		config->cfg |= mode << TRCCONFIGR_COND_SHIFT;
 	}
 
 	/* bit[11], Global timestamp tracing bit */
 	if ((config->mode & ETMv4_MODE_TIMESTAMP) && (drvdata->ts_size))
-		config->cfg |= BIT(11);
+		config->cfg |= TRCCONFIGR_TS;
 	else
-		config->cfg &= ~BIT(11);
+		config->cfg &= ~TRCCONFIGR_TS;
 
 	/* bit[12], Return stack enable bit */
 	if ((config->mode & ETM_MODE_RETURNSTACK) &&
 					(drvdata->retstack == true))
-		config->cfg |= BIT(12);
+		config->cfg |= TRCCONFIGR_RS;
 	else
-		config->cfg &= ~BIT(12);
+		config->cfg &= ~TRCCONFIGR_RS;
 
 	/* bits[14:13], Q element enable field */
 	mode = ETM_MODE_QELEM(config->mode);
 	/* start by clearing QE bits */
-	config->cfg &= ~(BIT(13) | BIT(14));
+	config->cfg &= ~(TRCCONFIGR_QE_W_COUNTS | TRCCONFIGR_QE_WO_COUNTS);
 	/*
 	 * if supported, Q elements with instruction counts are enabled.
 	 * Always set the low bit for any requested mode. Valid combos are
 	 * 0b00, 0b01 and 0b11.
 	 */
 	if (mode && drvdata->q_support)
-		config->cfg |= BIT(13);
+		config->cfg |= TRCCONFIGR_QE_W_COUNTS;
 	/*
 	 * if supported, Q elements with and without instruction
 	 * counts are enabled
 	 */
 	if ((mode & BIT(1)) && (drvdata->q_support & BIT(1)))
-		config->cfg |= BIT(14);
+		config->cfg |= TRCCONFIGR_QE_WO_COUNTS;
 
 	/* bit[11], AMBA Trace Bus (ATB) trigger enable bit */
 	if ((config->mode & ETM_MODE_ATB_TRIGGER) &&
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index ca6ed39ceaf7..55e756020a94 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -194,6 +194,23 @@
 #define TRCIDR5_NUMCNTR_SHIFT			28
 #define TRCIDR5_NUMCNTR_MASK			GENMASK(2, 0)
 
+#define TRCCONFIGR_INSTP0_LOAD			BIT(1)
+#define TRCCONFIGR_INSTP0_STORE			BIT(2)
+#define TRCCONFIGR_INSTP0_LOAD_STORE		(TRCCONFIGR_INSTP0_LOAD | TRCCONFIGR_INSTP0_STORE)
+#define TRCCONFIGR_BB				BIT(3)
+#define TRCCONFIGR_CCI				BIT(4)
+#define TRCCONFIGR_CID				BIT(6)
+#define TRCCONFIGR_VMID				BIT(7)
+#define TRCCONFIGR_COND_SHIFT			8
+#define TRCCONFIGR_COND_MASK			GENMASK(2, 0)
+#define TRCCONFIGR_TS				BIT(11)
+#define TRCCONFIGR_RS				BIT(12)
+#define TRCCONFIGR_QE_W_COUNTS			BIT(13)
+#define TRCCONFIGR_QE_WO_COUNTS			BIT(14)
+#define TRCCONFIGR_VMIDOPT			BIT(15)
+#define TRCCONFIGR_DA				BIT(16)
+#define TRCCONFIGR_DV				BIT(17)
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
-- 
2.28.0

[PATCH v2 07/15] coresight: Make ETM4x TRCEVENTCTL1R register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 .../coresight/coresight-etm4x-sysfs.c         | 25 +++++++++++--------
 drivers/hwtracing/coresight/coresight-etm4x.h |  9 +++++++
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index 4c29ab4464a0..cfa6f72a1e39 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -384,16 +384,16 @@ static ssize_t mode_store(struct device *dev,
 	/* bit[11], AMBA Trace Bus (ATB) trigger enable bit */
 	if ((config->mode & ETM_MODE_ATB_TRIGGER) &&
 	    (drvdata->atbtrig == true))
-		config->eventctrl1 |= BIT(11);
+		config->eventctrl1 |= TRCEVENTCTL1R_ATB;
 	else
-		config->eventctrl1 &= ~BIT(11);
+		config->eventctrl1 &= ~TRCEVENTCTL1R_ATB;
 
 	/* bit[12], Low-power state behavior override bit */
 	if ((config->mode & ETM_MODE_LPOVERRIDE) &&
 	    (drvdata->lpoverride == true))
-		config->eventctrl1 |= BIT(12);
+		config->eventctrl1 |= TRCEVENTCTL1R_LPOVERRIDE;
 	else
-		config->eventctrl1 &= ~BIT(12);
+		config->eventctrl1 &= ~TRCEVENTCTL1R_LPOVERRIDE;
 
 	/* bit[8], Instruction stall bit */
 	if ((config->mode & ETM_MODE_ISTALL_EN) && (drvdata->stallctl == true))
@@ -534,7 +534,7 @@ static ssize_t event_instren_show(struct device *dev,
 	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
 	struct etmv4_config *config = &drvdata->config;
 
-	val = BMVAL(config->eventctrl1, 0, 3);
+	val = REG_VAL(config->eventctrl1, TRCEVENTCTL1R_INSTEN);
 	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
 }
 
@@ -551,23 +551,28 @@ static ssize_t event_instren_store(struct device *dev,
 
 	spin_lock(&drvdata->spinlock);
 	/* start by clearing all instruction event enable bits */
-	config->eventctrl1 &= ~(BIT(0) | BIT(1) | BIT(2) | BIT(3));
+	config->eventctrl1 &= ~(TRCEVENTCTL1R_INSTEN_MASK << TRCEVENTCTL1R_INSTEN_SHIFT);
 	switch (drvdata->nr_event) {
 	case 0x0:
 		/* generate Event element for event 1 */
-		config->eventctrl1 |= val & BIT(1);
+		config->eventctrl1 |= val & TRCEVENTCTL1R_INSTEN_1;
 		break;
 	case 0x1:
 		/* generate Event element for event 1 and 2 */
-		config->eventctrl1 |= val & (BIT(0) | BIT(1));
+		config->eventctrl1 |= val & (TRCEVENTCTL1R_INSTEN_0 | TRCEVENTCTL1R_INSTEN_1);
 		break;
 	case 0x2:
 		/* generate Event element for event 1, 2 and 3 */
-		config->eventctrl1 |= val & (BIT(0) | BIT(1) | BIT(2));
+		config->eventctrl1 |= val & (TRCEVENTCTL1R_INSTEN_0 |
+					     TRCEVENTCTL1R_INSTEN_1 |
+					     TRCEVENTCTL1R_INSTEN_2);
 		break;
 	case 0x3:
 		/* generate Event element for all 4 events */
-		config->eventctrl1 |= val & 0xF;
+		config->eventctrl1 |= val & (TRCEVENTCTL1R_INSTEN_0 |
+					     TRCEVENTCTL1R_INSTEN_1 |
+					     TRCEVENTCTL1R_INSTEN_2 |
+					     TRCEVENTCTL1R_INSTEN_3);
 		break;
 	default:
 		break;
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 55e756020a94..eb72b81bbf85 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -211,6 +211,15 @@
 #define TRCCONFIGR_DA				BIT(16)
 #define TRCCONFIGR_DV				BIT(17)
 
+#define TRCEVENTCTL1R_INSTEN_SHIFT		0
+#define TRCEVENTCTL1R_INSTEN_MASK		GENMASK(3, 0)
+#define TRCEVENTCTL1R_INSTEN_0			BIT(0)
+#define TRCEVENTCTL1R_INSTEN_1			BIT(1)
+#define TRCEVENTCTL1R_INSTEN_2			BIT(2)
+#define TRCEVENTCTL1R_INSTEN_3			BIT(3)
+#define TRCEVENTCTL1R_ATB			BIT(11)
+#define TRCEVENTCTL1R_LPOVERRIDE		BIT(12)
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
-- 
2.28.0

[PATCH v2 08/15] coresight: Make ETM4x TRCSTALLCTLR register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 12 ++++++------
 drivers/hwtracing/coresight/coresight-etm4x.h       |  4 ++++
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index cfa6f72a1e39..d808eeae8b07 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -397,22 +397,22 @@ static ssize_t mode_store(struct device *dev,
 
 	/* bit[8], Instruction stall bit */
 	if ((config->mode & ETM_MODE_ISTALL_EN) && (drvdata->stallctl == true))
-		config->stall_ctrl |= BIT(8);
+		config->stall_ctrl |= TRCSTALLCTLR_ISTALL;
 	else
-		config->stall_ctrl &= ~BIT(8);
+		config->stall_ctrl &= ~TRCSTALLCTLR_ISTALL;
 
 	/* bit[10], Prioritize instruction trace bit */
 	if (config->mode & ETM_MODE_INSTPRIO)
-		config->stall_ctrl |= BIT(10);
+		config->stall_ctrl |= TRCSTALLCTLR_INSTPRIORITY;
 	else
-		config->stall_ctrl &= ~BIT(10);
+		config->stall_ctrl &= ~TRCSTALLCTLR_INSTPRIORITY;
 
 	/* bit[13], Trace overflow prevention bit */
 	if ((config->mode & ETM_MODE_NOOVERFLOW) &&
 		(drvdata->nooverflow == true))
-		config->stall_ctrl |= BIT(13);
+		config->stall_ctrl |= TRCSTALLCTLR_NOOVERFLOW;
 	else
-		config->stall_ctrl &= ~BIT(13);
+		config->stall_ctrl &= ~TRCSTALLCTLR_NOOVERFLOW;
 
 	/* bit[9] Start/stop logic control bit */
 	if (config->mode & ETM_MODE_VIEWINST_STARTSTOP)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index eb72b81bbf85..e37393934e0d 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -220,6 +220,10 @@
 #define TRCEVENTCTL1R_ATB			BIT(11)
 #define TRCEVENTCTL1R_LPOVERRIDE		BIT(12)
 
+#define TRCSTALLCTLR_ISTALL			BIT(8)
+#define TRCSTALLCTLR_INSTPRIORITY		BIT(10)
+#define TRCSTALLCTLR_NOOVERFLOW			BIT(13)
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
-- 
2.28.0

[PATCH v2 09/15] coresight: Make ETM4x TRCVICTLR register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 .../coresight/coresight-etm4x-core.c          | 10 +++---
 .../coresight/coresight-etm4x-sysfs.c         | 32 +++++++++----------
 drivers/hwtracing/coresight/coresight-etm4x.h | 26 +++++++--------
 3 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index dc936fb1cc74..2f7ea60fd003 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1208,7 +1208,7 @@ static inline u32 etm4_get_victlr_access_type(struct etmv4_config *config)
 /* Set ELx trace filter access in the TRCVICTLR register */
 static void etm4_set_victlr_access(struct etmv4_config *config)
 {
-	config->vinst_ctrl &= ~TRCVICTLR_EXLEVEL_MASK;
+	config->vinst_ctrl &= ~(TRCVICTLR_EXLEVEL_MASK << TRCVICTLR_EXLEVEL_SHIFT);
 	config->vinst_ctrl |= etm4_get_victlr_access_type(config);
 }
 
@@ -1228,7 +1228,7 @@ static void etm4_set_default_config(struct etmv4_config *config)
 	config->ts_ctrl = 0x0;
 
 	/* TRCVICTLR::EVENT = 0x01, select the always on logic */
-	config->vinst_ctrl = BIT(0);
+	config->vinst_ctrl = (0x01 & TRCVICTLR_EVENT_MASK) << TRCVICTLR_EVENT_SHIFT;
 
 	/* TRCVICTLR::EXLEVEL_NS:EXLEVELS: Set kernel / user filtering */
 	etm4_set_victlr_access(config);
@@ -1337,7 +1337,7 @@ static void etm4_set_default_filter(struct etmv4_config *config)
 	 * TRCVICTLR::SSSTATUS == 1, the start-stop logic is
 	 * in the started state
 	 */
-	config->vinst_ctrl |= BIT(9);
+	config->vinst_ctrl |= TRCVICTLR_SSSTATUS;
 	config->mode |= ETM_MODE_VIEWINST_STARTSTOP;
 
 	/* No start-stop filtering for ViewInst */
@@ -1441,7 +1441,7 @@ static int etm4_set_event_filters(struct etmv4_drvdata *drvdata,
 			 * TRCVICTLR::SSSTATUS == 1, the start-stop logic is
 			 * in the started state
 			 */
-			config->vinst_ctrl |= BIT(9);
+			config->vinst_ctrl |= TRCVICTLR_SSSTATUS;
 
 			/* No start-stop filtering for ViewInst */
 			config->vissctlr = 0x0;
@@ -1469,7 +1469,7 @@ static int etm4_set_event_filters(struct etmv4_drvdata *drvdata,
 			 * etm4_disable_perf().
 			 */
 			if (filters->ssstatus)
-				config->vinst_ctrl |= BIT(9);
+				config->vinst_ctrl |= TRCVICTLR_SSSTATUS;
 
 			/* No include/exclude filtering for ViewInst */
 			config->viiectlr = 0x0;
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index d808eeae8b07..87e52f685f05 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -206,11 +206,11 @@ static ssize_t reset_store(struct device *dev,
 	 * started state. ARM recommends start-stop logic is set before
 	 * each trace run.
 	 */
-	config->vinst_ctrl = BIT(0);
+	config->vinst_ctrl = (0x01 & TRCVICTLR_EVENT_MASK) << TRCVICTLR_EVENT_SHIFT;
 	if (drvdata->nr_addr_cmp > 0) {
 		config->mode |= ETM_MODE_VIEWINST_STARTSTOP;
 		/* SSSTATUS, bit[9] */
-		config->vinst_ctrl |= BIT(9);
+		config->vinst_ctrl |= TRCVICTLR_SSSTATUS;
 	}
 
 	/* No address range filtering for ViewInst */
@@ -416,22 +416,22 @@ static ssize_t mode_store(struct device *dev,
 
 	/* bit[9] Start/stop logic control bit */
 	if (config->mode & ETM_MODE_VIEWINST_STARTSTOP)
-		config->vinst_ctrl |= BIT(9);
+		config->vinst_ctrl |= TRCVICTLR_SSSTATUS;
 	else
-		config->vinst_ctrl &= ~BIT(9);
+		config->vinst_ctrl &= ~TRCVICTLR_SSSTATUS;
 
 	/* bit[10], Whether a trace unit must trace a Reset exception */
 	if (config->mode & ETM_MODE_TRACE_RESET)
-		config->vinst_ctrl |= BIT(10);
+		config->vinst_ctrl |= TRCVICTLR_TRCRESET;
 	else
-		config->vinst_ctrl &= ~BIT(10);
+		config->vinst_ctrl &= ~TRCVICTLR_TRCRESET;
 
 	/* bit[11], Whether a trace unit must trace a system error exception */
 	if ((config->mode & ETM_MODE_TRACE_ERR) &&
 		(drvdata->trc_error == true))
-		config->vinst_ctrl |= BIT(11);
+		config->vinst_ctrl |= TRCVICTLR_TRCERR;
 	else
-		config->vinst_ctrl &= ~BIT(11);
+		config->vinst_ctrl &= ~TRCVICTLR_TRCERR;
 
 	if (config->mode & (ETM_MODE_EXCL_KERN | ETM_MODE_EXCL_USER))
 		etm4_config_trace_mode(config);
@@ -723,7 +723,7 @@ static ssize_t event_vinst_show(struct device *dev,
 	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
 	struct etmv4_config *config = &drvdata->config;
 
-	val = config->vinst_ctrl & ETMv4_EVENT_MASK;
+	val = REG_VAL(config->vinst_ctrl, TRCVICTLR_EVENT);
 	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
 }
 
@@ -739,9 +739,9 @@ static ssize_t event_vinst_store(struct device *dev,
 		return -EINVAL;
 
 	spin_lock(&drvdata->spinlock);
-	val &= ETMv4_EVENT_MASK;
-	config->vinst_ctrl &= ~ETMv4_EVENT_MASK;
-	config->vinst_ctrl |= val;
+	val &= TRCVICTLR_EVENT_MASK;
+	config->vinst_ctrl &= ~(TRCVICTLR_EVENT_MASK << TRCVICTLR_EVENT_SHIFT);
+	config->vinst_ctrl |= val << TRCVICTLR_EVENT_SHIFT;
 	spin_unlock(&drvdata->spinlock);
 	return size;
 }
@@ -755,7 +755,7 @@ static ssize_t s_exlevel_vinst_show(struct device *dev,
 	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
 	struct etmv4_config *config = &drvdata->config;
 
-	val = (config->vinst_ctrl & TRCVICTLR_EXLEVEL_S_MASK) >> TRCVICTLR_EXLEVEL_S_SHIFT;
+	val = REG_VAL(config->vinst_ctrl, TRCVICTLR_EXLEVEL_S);
 	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
 }
 
@@ -772,7 +772,7 @@ static ssize_t s_exlevel_vinst_store(struct device *dev,
 
 	spin_lock(&drvdata->spinlock);
 	/* clear all EXLEVEL_S bits  */
-	config->vinst_ctrl &= ~(TRCVICTLR_EXLEVEL_S_MASK);
+	config->vinst_ctrl &= ~(TRCVICTLR_EXLEVEL_S_MASK << TRCVICTLR_EXLEVEL_S_SHIFT);
 	/* enable instruction tracing for corresponding exception level */
 	val &= drvdata->s_ex_level;
 	config->vinst_ctrl |= (val << TRCVICTLR_EXLEVEL_S_SHIFT);
@@ -790,7 +790,7 @@ static ssize_t ns_exlevel_vinst_show(struct device *dev,
 	struct etmv4_config *config = &drvdata->config;
 
 	/* EXLEVEL_NS, bits[23:20] */
-	val = (config->vinst_ctrl & TRCVICTLR_EXLEVEL_NS_MASK) >> TRCVICTLR_EXLEVEL_NS_SHIFT;
+	val = REG_VAL(config->vinst_ctrl, TRCVICTLR_EXLEVEL_NS);
 	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
 }
 
@@ -807,7 +807,7 @@ static ssize_t ns_exlevel_vinst_store(struct device *dev,
 
 	spin_lock(&drvdata->spinlock);
 	/* clear EXLEVEL_NS bits  */
-	config->vinst_ctrl &= ~(TRCVICTLR_EXLEVEL_NS_MASK);
+	config->vinst_ctrl &= ~(TRCVICTLR_EXLEVEL_NS_MASK << TRCVICTLR_EXLEVEL_NS_SHIFT);
 	/* enable instruction tracing for corresponding exception level */
 	val &= drvdata->ns_ex_level;
 	config->vinst_ctrl |= (val << TRCVICTLR_EXLEVEL_NS_SHIFT);
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index e37393934e0d..02afce9dcf6b 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -224,6 +224,18 @@
 #define TRCSTALLCTLR_INSTPRIORITY		BIT(10)
 #define TRCSTALLCTLR_NOOVERFLOW			BIT(13)
 
+#define TRCVICTLR_EVENT_SHIFT			0
+#define TRCVICTLR_EVENT_MASK			GENMASK(7, 0)
+#define TRCVICTLR_SSSTATUS			BIT(9)
+#define TRCVICTLR_TRCRESET			BIT(10)
+#define TRCVICTLR_TRCERR			BIT(11)
+#define TRCVICTLR_EXLEVEL_SHIFT			16
+#define TRCVICTLR_EXLEVEL_MASK			GENMASK(6, 0)
+#define TRCVICTLR_EXLEVEL_S_SHIFT		16
+#define TRCVICTLR_EXLEVEL_S_MASK		GENMASK(3, 0)
+#define TRCVICTLR_EXLEVEL_NS_SHIFT		20
+#define TRCVICTLR_EXLEVEL_NS_MASK		GENMASK(2, 0)
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
@@ -724,23 +736,9 @@
 #define ETM_EXLEVEL_NS_OS		BIT(5)	/* NonSecure EL1	*/
 #define ETM_EXLEVEL_NS_HYP		BIT(6)	/* NonSecure EL2	*/
 
-#define ETM_EXLEVEL_MASK		(GENMASK(6, 0))
-#define ETM_EXLEVEL_S_MASK		(GENMASK(3, 0))
-#define ETM_EXLEVEL_NS_MASK		(GENMASK(6, 4))
-
 /* access level controls in TRCACATRn */
 #define TRCACATR_EXLEVEL_SHIFT		8
 
-/* access level control in TRCVICTLR */
-#define TRCVICTLR_EXLEVEL_SHIFT		16
-#define TRCVICTLR_EXLEVEL_S_SHIFT	16
-#define TRCVICTLR_EXLEVEL_NS_SHIFT	20
-
-/* secure / non secure masks - TRCVICTLR, IDR3 */
-#define TRCVICTLR_EXLEVEL_MASK		(ETM_EXLEVEL_MASK << TRCVICTLR_EXLEVEL_SHIFT)
-#define TRCVICTLR_EXLEVEL_S_MASK	(ETM_EXLEVEL_S_MASK << TRCVICTLR_EXLEVEL_SHIFT)
-#define TRCVICTLR_EXLEVEL_NS_MASK	(ETM_EXLEVEL_NS_MASK << TRCVICTLR_EXLEVEL_SHIFT)
-
 #define ETM_TRCIDR1_ARCH_MAJOR_SHIFT	8
 #define ETM_TRCIDR1_ARCH_MAJOR_MASK	(0xfU << ETM_TRCIDR1_ARCH_MAJOR_SHIFT)
 #define ETM_TRCIDR1_ARCH_MAJOR(x)	\
-- 
2.28.0

[PATCH v2 10/15] coresight: Make ETM3x ETMTECR1 register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm3x-core.c  | 2 +-
 drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
index cf64ce73a741..35e4bdee848c 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -204,7 +204,7 @@ void etm_set_default(struct etm_config *config)
 	 *  set all bits in register 0x007, the ETMTECR2, to 0
 	 *  set register 0x008, the ETMTEEVR, to 0x6F (TRUE).
 	 */
-	config->enable_ctrl1 = BIT(24);
+	config->enable_ctrl1 = ETMTECR1_INC_EXC;
 	config->enable_ctrl2 = 0x0;
 	config->enable_event = ETM_HARD_WIRE_RES_A;
 
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
index e8c7649f123e..68fcbf4ce7a8 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
@@ -474,7 +474,7 @@ static ssize_t addr_start_store(struct device *dev,
 	config->addr_val[idx] = val;
 	config->addr_type[idx] = ETM_ADDR_TYPE_START;
 	config->startstop_ctrl |= (1 << idx);
-	config->enable_ctrl1 |= BIT(25);
+	config->enable_ctrl1 |= ETMTECR1_START_STOP;
 	spin_unlock(&drvdata->spinlock);
 
 	return size;
-- 
2.28.0

[PATCH v2 11/15] coresight: Make ETM4x TRCACATRn register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 .../coresight/coresight-etm4x-sysfs.c         | 43 ++++++++++---------
 drivers/hwtracing/coresight/coresight-etm4x.h | 18 ++++++--
 2 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index 87e52f685f05..51f6e13e3b29 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -22,7 +22,7 @@ static int etm4_set_mode_exclude(struct etmv4_drvdata *drvdata, bool exclude)
 	 * TRCACATRn.TYPE bit[1:0]: type of comparison
 	 * the trace unit performs
 	 */
-	if (BMVAL(config->addr_acc[idx], 0, 1) == ETM_INSTR_ADDR) {
+	if (REG_VAL(config->addr_acc[idx], TRCACATRn_TYPE) == TRCACATRn_TYPE_ADDR) {
 		if (idx % 2 != 0)
 			return -EINVAL;
 
@@ -863,11 +863,11 @@ static ssize_t addr_instdatatype_show(struct device *dev,
 
 	spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
-	val = BMVAL(config->addr_acc[idx], 0, 1);
+	val = REG_VAL(config->addr_acc[idx], TRCACATRn_TYPE);
 	len = scnprintf(buf, PAGE_SIZE, "%s\n",
-			val == ETM_INSTR_ADDR ? "instr" :
-			(val == ETM_DATA_LOAD_ADDR ? "data_load" :
-			(val == ETM_DATA_STORE_ADDR ? "data_store" :
+			val == TRCACATRn_TYPE_ADDR ? "instr" :
+			(val == TRCACATRn_TYPE_DATA_LOAD_ADDR ? "data_load" :
+			(val == TRCACATRn_TYPE_DATA_STORE_ADDR ? "data_store" :
 			"data_load_store")));
 	spin_unlock(&drvdata->spinlock);
 	return len;
@@ -891,7 +891,7 @@ static ssize_t addr_instdatatype_store(struct device *dev,
 	idx = config->addr_idx;
 	if (!strcmp(str, "instr"))
 		/* TYPE, bits[1:0] */
-		config->addr_acc[idx] &= ~(BIT(0) | BIT(1));
+		config->addr_acc[idx] &= ~(TRCACATRn_TYPE_MASK << TRCACATRn_TYPE_SHIFT);
 
 	spin_unlock(&drvdata->spinlock);
 	return size;
@@ -1149,7 +1149,7 @@ static ssize_t addr_ctxtype_show(struct device *dev,
 	spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	/* CONTEXTTYPE, bits[3:2] */
-	val = BMVAL(config->addr_acc[idx], 2, 3);
+	val = REG_VAL(config->addr_acc[idx], TRCACATRn_CONTEXTTYPE);
 	len = scnprintf(buf, PAGE_SIZE, "%s\n", val == ETM_CTX_NONE ? "none" :
 			(val == ETM_CTX_CTXID ? "ctxid" :
 			(val == ETM_CTX_VMID ? "vmid" : "all")));
@@ -1175,18 +1175,19 @@ static ssize_t addr_ctxtype_store(struct device *dev,
 	idx = config->addr_idx;
 	if (!strcmp(str, "none"))
 		/* start by clearing context type bits */
-		config->addr_acc[idx] &= ~(BIT(2) | BIT(3));
+		config->addr_acc[idx] &= ~(TRCACATRn_CONTEXTTYPE_MASK <<
+					   TRCACATRn_CONTEXTTYPE_SHIFT);
 	else if (!strcmp(str, "ctxid")) {
 		/* 0b01 The trace unit performs a Context ID */
 		if (drvdata->numcidc) {
-			config->addr_acc[idx] |= BIT(2);
-			config->addr_acc[idx] &= ~BIT(3);
+			config->addr_acc[idx] |= TRCACATRn_CONTEXTTYPE_CTXID;
+			config->addr_acc[idx] &= ~TRCACATRn_CONTEXTTYPE_VMID;
 		}
 	} else if (!strcmp(str, "vmid")) {
 		/* 0b10 The trace unit performs a VMID */
 		if (drvdata->numvmidc) {
-			config->addr_acc[idx] &= ~BIT(2);
-			config->addr_acc[idx] |= BIT(3);
+			config->addr_acc[idx] &= ~TRCACATRn_CONTEXTTYPE_CTXID;
+			config->addr_acc[idx] |= TRCACATRn_CONTEXTTYPE_VMID;
 		}
 	} else if (!strcmp(str, "all")) {
 		/*
@@ -1194,9 +1195,9 @@ static ssize_t addr_ctxtype_store(struct device *dev,
 		 * comparison and a VMID
 		 */
 		if (drvdata->numcidc)
-			config->addr_acc[idx] |= BIT(2);
+			config->addr_acc[idx] |= TRCACATRn_CONTEXTTYPE_CTXID;
 		if (drvdata->numvmidc)
-			config->addr_acc[idx] |= BIT(3);
+			config->addr_acc[idx] |= TRCACATRn_CONTEXTTYPE_VMID;
 	}
 	spin_unlock(&drvdata->spinlock);
 	return size;
@@ -1215,7 +1216,7 @@ static ssize_t addr_context_show(struct device *dev,
 	spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	/* context ID comparator bits[6:4] */
-	val = BMVAL(config->addr_acc[idx], 4, 6);
+	val = REG_VAL(config->addr_acc[idx], TRCACATRn_CONTEXT);
 	spin_unlock(&drvdata->spinlock);
 	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
 }
@@ -1240,8 +1241,8 @@ static ssize_t addr_context_store(struct device *dev,
 	spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	/* clear context ID comparator bits[6:4] */
-	config->addr_acc[idx] &= ~(BIT(4) | BIT(5) | BIT(6));
-	config->addr_acc[idx] |= (val << 4);
+	config->addr_acc[idx] &= ~(TRCACATRn_CONTEXT_MASK << TRCACATRn_CONTEXT_SHIFT);
+	config->addr_acc[idx] |= (val << TRCACATRn_CONTEXT_SHIFT);
 	spin_unlock(&drvdata->spinlock);
 	return size;
 }
@@ -1258,7 +1259,7 @@ static ssize_t addr_exlevel_s_ns_show(struct device *dev,
 
 	spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
-	val = BMVAL(config->addr_acc[idx], 8, 14);
+	val = REG_VAL(config->addr_acc[idx], TRCACATRn_EXLEVEL);
 	spin_unlock(&drvdata->spinlock);
 	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
 }
@@ -1275,14 +1276,14 @@ static ssize_t addr_exlevel_s_ns_store(struct device *dev,
 	if (kstrtoul(buf, 0, &val))
 		return -EINVAL;
 
-	if (val & ~((GENMASK(14, 8) >> 8)))
+	if (val & ~TRCACATRn_EXLEVEL_MASK)
 		return -EINVAL;
 
 	spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	/* clear Exlevel_ns & Exlevel_s bits[14:12, 11:8], bit[15] is res0 */
-	config->addr_acc[idx] &= ~(GENMASK(14, 8));
-	config->addr_acc[idx] |= (val << 8);
+	config->addr_acc[idx] &= ~(TRCACATRn_EXLEVEL_MASK << TRCACATRn_EXLEVEL_SHIFT);
+	config->addr_acc[idx] |= (val << TRCACATRn_EXLEVEL_SHIFT);
 	spin_unlock(&drvdata->spinlock);
 	return size;
 }
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 02afce9dcf6b..5701d970d81a 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -236,6 +236,16 @@
 #define TRCVICTLR_EXLEVEL_NS_SHIFT		20
 #define TRCVICTLR_EXLEVEL_NS_MASK		GENMASK(2, 0)
 
+#define TRCACATRn_TYPE_SHIFT			0
+#define TRCACATRn_TYPE_MASK			GENMASK(1, 0)
+#define TRCACATRn_CONTEXTTYPE_SHIFT		2
+#define TRCACATRn_CONTEXTTYPE_MASK		GENMASK(1, 0)
+#define TRCACATRn_CONTEXTTYPE_CTXID		BIT(2)
+#define TRCACATRn_CONTEXTTYPE_VMID		BIT(3)
+#define TRCACATRn_CONTEXT_SHIFT			4
+#define TRCACATRn_CONTEXT_MASK			GENMASK(2, 0)
+#define TRCACATRn_EXLEVEL_SHIFT			8
+#define TRCACATRn_EXLEVEL_MASK			GENMASK(6, 0)
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
@@ -1078,10 +1088,10 @@ struct etmv4_drvdata {
 
 /* Address comparator access types */
 enum etm_addr_acctype {
-	ETM_INSTR_ADDR,
-	ETM_DATA_LOAD_ADDR,
-	ETM_DATA_STORE_ADDR,
-	ETM_DATA_LOAD_STORE_ADDR,
+	TRCACATRn_TYPE_ADDR,
+	TRCACATRn_TYPE_DATA_LOAD_ADDR,
+	TRCACATRn_TYPE_DATA_STORE_ADDR,
+	TRCACATRn_TYPE_DATA_LOAD_STORE_ADDR,
 };
 
 /* Address comparator context types */
-- 
2.28.0

[PATCH v2 12/15] coresight: Make ETM4x TRCSSCCRn and TRCSSCSRn register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x-core.c  | 2 +-
 drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 6 +++---
 drivers/hwtracing/coresight/coresight-etm4x.h       | 5 +++++
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 2f7ea60fd003..e134bdabebcf 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -443,7 +443,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
 	for (i = 0; i < drvdata->nr_ss_cmp; i++) {
 		/* always clear status bit on restart if using single-shot */
 		if (config->ss_ctrl[i] || config->ss_pe_cmp[i])
-			config->ss_status[i] &= ~BIT(31);
+			config->ss_status[i] &= ~TRCSSCSRn_STATUS;
 		etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i));
 		etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i));
 		if (etm4x_sspcicrn_present(drvdata, i))
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index 51f6e13e3b29..7d9372ba1168 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -1793,9 +1793,9 @@ static ssize_t sshot_ctrl_store(struct device *dev,
 
 	spin_lock(&drvdata->spinlock);
 	idx = config->ss_idx;
-	config->ss_ctrl[idx] = val & GENMASK(24, 0);
+	config->ss_ctrl[idx] = val & (TRCSSCCRn_SAC_ARC_RST_MASK << TRCSSCCRn_SAC_ARC_RST_SHIFT);
 	/* must clear bit 31 in related status register on programming */
-	config->ss_status[idx] &= ~BIT(31);
+	config->ss_status[idx] &= ~TRCSSCSRn_STATUS;
 	spin_unlock(&drvdata->spinlock);
 	return size;
 }
@@ -1845,7 +1845,7 @@ static ssize_t sshot_pe_ctrl_store(struct device *dev,
 	idx = config->ss_idx;
 	config->ss_pe_cmp[idx] = val & GENMASK(7, 0);
 	/* must clear bit 31 in related status register on programming */
-	config->ss_status[idx] &= ~BIT(31);
+	config->ss_status[idx] &= ~TRCSSCSRn_STATUS;
 	spin_unlock(&drvdata->spinlock);
 	return size;
 }
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 5701d970d81a..9c22a5b0777f 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -246,6 +246,11 @@
 #define TRCACATRn_CONTEXT_MASK			GENMASK(2, 0)
 #define TRCACATRn_EXLEVEL_SHIFT			8
 #define TRCACATRn_EXLEVEL_MASK			GENMASK(6, 0)
+
+#define TRCSSCSRn_STATUS			BIT(31)
+#define TRCSSCCRn_SAC_ARC_RST_SHIFT		0
+#define TRCSSCCRn_SAC_ARC_RST_MASK		GENMASK(24, 0)
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
-- 
2.28.0

[PATCH v2 13/15] coresight: Make ETM4x TRCSSPCICRn register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 2 +-
 drivers/hwtracing/coresight/coresight-etm4x.h       | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index 7d9372ba1168..682819467755 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -1843,7 +1843,7 @@ static ssize_t sshot_pe_ctrl_store(struct device *dev,
 
 	spin_lock(&drvdata->spinlock);
 	idx = config->ss_idx;
-	config->ss_pe_cmp[idx] = val & GENMASK(7, 0);
+	config->ss_pe_cmp[idx] = val & (TRCSSPCICRn_PC_MASK << TRCSSPCICRn_PC_SHIFT);
 	/* must clear bit 31 in related status register on programming */
 	config->ss_status[idx] &= ~TRCSSCSRn_STATUS;
 	spin_unlock(&drvdata->spinlock);
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 9c22a5b0777f..9d0978540338 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -251,6 +251,9 @@
 #define TRCSSCCRn_SAC_ARC_RST_SHIFT		0
 #define TRCSSCCRn_SAC_ARC_RST_MASK		GENMASK(24, 0)
 
+#define TRCSSPCICRn_PC_SHIFT			0
+#define TRCSSPCICRn_PC_MASK			GENMASK(7, 0)
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
-- 
2.28.0

[PATCH v2 14/15] coresight: Make ETM4x TRCBBCTLR register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 5 +++--
 drivers/hwtracing/coresight/coresight-etm4x.h       | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index 682819467755..a0cdd2cd978a 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -707,10 +707,11 @@ static ssize_t bb_ctrl_store(struct device *dev,
 	 * individual range comparators. If include then at least 1
 	 * range must be selected.
 	 */
-	if ((val & BIT(8)) && (BMVAL(val, 0, 7) == 0))
+	if ((val & TRCBBCTLR_MODE) && (REG_VAL(val, TRCBBCTLR_RANGE) == 0))
 		return -EINVAL;
 
-	config->bb_ctrl = val & GENMASK(8, 0);
+	config->bb_ctrl = val & (TRCBBCTLR_MODE |
+				 (TRCBBCTLR_RANGE_MASK << TRCBBCTLR_RANGE_SHIFT));
 	return size;
 }
 static DEVICE_ATTR_RW(bb_ctrl);
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 9d0978540338..4d943faade33 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -254,6 +254,10 @@
 #define TRCSSPCICRn_PC_SHIFT			0
 #define TRCSSPCICRn_PC_MASK			GENMASK(7, 0)
 
+#define TRCBBCTLR_MODE				BIT(8)
+#define TRCBBCTLR_RANGE_SHIFT			0
+#define TRCBBCTLR_RANGE_MASK			GENMASK(7, 0)
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
-- 
2.28.0

[PATCH v2 15/15] coresight: Make ETM4x TRCRSCTLRn register accesses consistent with sysreg.h

[James Clark]

This is a no-op change for style and consistency and has no effect on the
binary produced by gcc-11.

Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 7 +++++--
 drivers/hwtracing/coresight/coresight-etm4x.h       | 9 +++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index a0cdd2cd978a..c876a63fa84d 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -1728,8 +1728,11 @@ static ssize_t res_ctrl_store(struct device *dev,
 	/* For odd idx pair inversal bit is RES0 */
 	if (idx % 2 != 0)
 		/* PAIRINV, bit[21] */
-		val &= ~BIT(21);
-	config->res_ctrl[idx] = val & GENMASK(21, 0);
+		val &= ~TRCRSCTLRn_PAIRINV;
+	config->res_ctrl[idx] = val & (TRCRSCTLRn_PAIRINV |
+				       TRCRSCTLRn_INV |
+				       (TRCRSCTLRn_GROUP_MASK << TRCRSCTLRn_GROUP_SHIFT) |
+				       (TRCRSCTLRn_SELECT_MASK << TRCRSCTLRn_SELECT_SHIFT));
 	spin_unlock(&drvdata->spinlock);
 	return size;
 }
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 4d943faade33..dd2156a5e70b 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -258,6 +258,15 @@
 #define TRCBBCTLR_RANGE_SHIFT			0
 #define TRCBBCTLR_RANGE_MASK			GENMASK(7, 0)
 
+#define TRCRSCTLRn_PAIRINV			BIT(21)
+#define TRCRSCTLRn_INV				BIT(20)
+#define TRCRSCTLRn_GROUP_SHIFT			16
+#define TRCRSCTLRn_GROUP_MASK			GENMASK(3, 0)
+#define TRCRSCTLRn_SELECT_SHIFT			0
+#define TRCRSCTLRn_SELECT_MASK			GENMASK(15, 0)
+
+
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
-- 
2.28.0

Re: [PATCH v2 01/15] coresight: Make ETM4x TRCIDR0 register accesses consistent with sysreg.h

[Anshuman Khandual]

Hi James,

These are all ETM4X specific changes. Something like this might be cleaner
and also more compact. Also would suggest to follow the same for subsequent
patches as well.

coresight: etm4x: Cleanup TRCIDR0 register accesses

Consistency with sysreg.h could be mentioned in the commit message itself.

On 2/3/22 5:35 PM, James Clark wrote:
> This is a no-op change for style and consistency and has no effect on the
> binary produced by gcc-11.

This patch adds register definitions, helper macros as well. Please expand
the commit message to add more details. This is too short, for the change
it creates. BTW why is it necessary to mention GCC version number here.

> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  .../coresight/coresight-etm4x-core.c          | 36 +++++--------------
>  drivers/hwtracing/coresight/coresight-etm4x.h | 17 +++++++++
>  drivers/hwtracing/coresight/coresight-priv.h  |  5 +++
>  3 files changed, 30 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index e2eebd865241..107e81948f76 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1091,41 +1091,21 @@ static void etm4_init_arch_data(void *info)
>  	etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
>  
>  	/* INSTP0, bits[2:1] P0 tracing support field */
> -	if (BMVAL(etmidr0, 1, 2) == 0b11)
> -		drvdata->instrp0 = true;
> -	else
> -		drvdata->instrp0 = false;
> -
> +	drvdata->instrp0 = !!(REG_VAL(etmidr0, TRCIDR0_INSTP0) == 0b11);
>  	/* TRCBB, bit[5] Branch broadcast tracing support bit */
> -	if (BMVAL(etmidr0, 5, 5))
> -		drvdata->trcbb = true;
> -	else
> -		drvdata->trcbb = false;
> -
> +	drvdata->trcbb = !!(etmidr0 & TRCIDR0_TRCBB);
>  	/* TRCCOND, bit[6] Conditional instruction tracing support bit */
> -	if (BMVAL(etmidr0, 6, 6))
> -		drvdata->trccond = true;
> -	else
> -		drvdata->trccond = false;
> -
> +	drvdata->trccond = !!(etmidr0 & TRCIDR0_TRCCOND);
>  	/* TRCCCI, bit[7] Cycle counting instruction bit */
> -	if (BMVAL(etmidr0, 7, 7))
> -		drvdata->trccci = true;
> -	else
> -		drvdata->trccci = false;
> -
> +	drvdata->trccci = !!(etmidr0 & TRCIDR0_TRCCCI);
>  	/* RETSTACK, bit[9] Return stack bit */
> -	if (BMVAL(etmidr0, 9, 9))
> -		drvdata->retstack = true;
> -	else
> -		drvdata->retstack = false;
> -
> +	drvdata->retstack = !!(etmidr0 & TRCIDR0_RETSTACK);
>  	/* NUMEVENT, bits[11:10] Number of events field */
> -	drvdata->nr_event = BMVAL(etmidr0, 10, 11);
> +	drvdata->nr_event = REG_VAL(etmidr0, TRCIDR0_NUMEVENT);
>  	/* QSUPP, bits[16:15] Q element support field */
> -	drvdata->q_support = BMVAL(etmidr0, 15, 16);
> +	drvdata->q_support = REG_VAL(etmidr0, TRCIDR0_QSUPP);
>  	/* TSSIZE, bits[28:24] Global timestamp size field */
> -	drvdata->ts_size = BMVAL(etmidr0, 24, 28);
> +	drvdata->ts_size = REG_VAL(etmidr0, TRCIDR0_TSSIZE);
>  
>  	/* maximum size of resources */
>  	etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 3c4d69b096ca..2bd8ad953b8e 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -130,6 +130,23 @@
>  
>  #define TRCRSR_TA			BIT(12)
>  
> +/*
> + * Bit positions of registers that are defined above, in the sysreg.h style
> + * of _MASK, _SHIFT and BIT().
> + */

^^^ not really necessary. Instead the format requirement for below mentioned
CORESIGHT_REG_VAL() macro might be relevant and should be mentioned.

> +#define TRCIDR0_INSTP0_SHIFT			1
> +#define TRCIDR0_INSTP0_MASK			GENMASK(1, 0)
> +#define TRCIDR0_TRCBB				BIT(5)
> +#define TRCIDR0_TRCCOND				BIT(6)
> +#define TRCIDR0_TRCCCI				BIT(7)
> +#define TRCIDR0_RETSTACK			BIT(9)
> +#define TRCIDR0_NUMEVENT_SHIFT			10
> +#define TRCIDR0_NUMEVENT_MASK			GENMASK(1, 0)
> +#define TRCIDR0_QSUPP_SHIFT			15
> +#define TRCIDR0_QSUPP_MASK			GENMASK(1, 0)
> +#define TRCIDR0_TSSIZE_SHIFT			24
> +#define TRCIDR0_TSSIZE_MASK			GENMASK(4, 0)
> +
>  /*
>   * System instructions to access ETM registers.
>   * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index ff1dd2092ac5..1452c6038421 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -36,6 +36,11 @@
>  
>  #define TIMEOUT_US		100
>  #define BMVAL(val, lsb, msb)	((val & GENMASK(msb, lsb)) >> lsb)
> +/*
> + * Extract a field from a register where field is #defined in the form
> + * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
> + */

Looking at the usage, <register_name> is already embedded in <filed_name>. So
it requires <field_name>_SHIFT and <field_name>_MASK instead. Unless register
name should be passed as separate argument (which actually might be better).

REG_VAL(etmidr0, TRCIDR0_TSSIZE) ----> REG_VAL(etmidr0, TRCIDR0, TSSIZE)

with some restructuring in the comment ..

/*
 * Extract a field from a coresight register
 *
 * Required fields are defined as macros like the following
 *  
 * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
 */

> +#define REG_VAL(val, field)	((val & (field##_MASK << field##_SHIFT)) >> field##_SHIFT)

This is too generic to be in a coresight header or it should just be
named CORESIGHT_REG_VAL() instead, making it more specific for here.

The build should fail in case any required macro definition is absent.
I guess no more fortification is required in case macros are missing.

However CORESIGHT_REG_VAL() is better placed in <coresight-etm4x.h>
just before all the dependent SHIFT/MASK register field definition
starts.

>  
>  #define ETM_MODE_EXCL_KERN	BIT(30)
>  #define ETM_MODE_EXCL_USER	BIT(31)
> 

- Anshuman

Re: [PATCH v2 00/15] Make ETM register accesses consistent with sysreg.h

[Anshuman Khandual]

Hi James,

On 2/3/22 5:35 PM, James Clark wrote:
> James Clark (15):
>   coresight: Make ETM4x TRCIDR0 register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCIDR2 register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCIDR3 register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCIDR4 register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCIDR5 register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCCONFIGR register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCEVENTCTL1R register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCSTALLCTLR register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCVICTLR register accesses consistent with
>     sysreg.h
>   coresight: Make ETM3x ETMTECR1 register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCACATRn register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCSSCCRn and TRCSSCSRn register accesses
>     consistent with sysreg.h
>   coresight: Make ETM4x TRCSSPCICRn register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCBBCTLR register accesses consistent with
>     sysreg.h
>   coresight: Make ETM4x TRCRSCTLRn register accesses consistent with
>     sysreg.h

The changes here are very similar to each other. But they are split
into different patches according to register names just for better
review process ? OR is there any other rationale ?

- Anshuman

Re: [PATCH v2 00/15] Make ETM register accesses consistent with sysreg.h

[James Clark]

On 07/02/2022 05:51, Anshuman Khandual wrote:
> Hi James,
> 
> On 2/3/22 5:35 PM, James Clark wrote:
>> James Clark (15):
>>   coresight: Make ETM4x TRCIDR0 register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCIDR2 register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCIDR3 register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCIDR4 register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCIDR5 register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCCONFIGR register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCEVENTCTL1R register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCSTALLCTLR register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCVICTLR register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM3x ETMTECR1 register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCACATRn register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCSSCCRn and TRCSSCSRn register accesses
>>     consistent with sysreg.h
>>   coresight: Make ETM4x TRCSSPCICRn register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCBBCTLR register accesses consistent with
>>     sysreg.h
>>   coresight: Make ETM4x TRCRSCTLRn register accesses consistent with
>>     sysreg.h
> 
> The changes here are very similar to each other. But they are split
> into different patches according to register names just for better
> review process ? OR is there any other rationale ?

Yes just for the review process. I didn't see a way of reviewing them all
in one change because it's so big, and the only logical way to split it was
by register so I did it that way.

> 
> - Anshuman

Re: [PATCH v2 01/15] coresight: Make ETM4x TRCIDR0 register accesses consistent with sysreg.h

[Mathieu Poirier]

On Mon, Feb 07, 2022 at 11:14:50AM +0530, Anshuman Khandual wrote:
> Hi James,
> 
> These are all ETM4X specific changes. Something like this might be cleaner
> and also more compact. Also would suggest to follow the same for subsequent
> patches as well.
> 
> coresight: etm4x: Cleanup TRCIDR0 register accesses
> 
> Consistency with sysreg.h could be mentioned in the commit message itself.
>

I agree with the above two comments.

> On 2/3/22 5:35 PM, James Clark wrote:
> > This is a no-op change for style and consistency and has no effect on the
> > binary produced by gcc-11.
> 
> This patch adds register definitions, helper macros as well. Please expand
> the commit message to add more details. This is too short, for the change
> it creates. BTW why is it necessary to mention GCC version number here.
> 

Here too - I'm not sure adding gcc's version number helps in any way.

> > 
> > Signed-off-by: James Clark <james.clark@arm.com>
> > ---
> >  .../coresight/coresight-etm4x-core.c          | 36 +++++--------------
> >  drivers/hwtracing/coresight/coresight-etm4x.h | 17 +++++++++
> >  drivers/hwtracing/coresight/coresight-priv.h  |  5 +++
> >  3 files changed, 30 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index e2eebd865241..107e81948f76 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -1091,41 +1091,21 @@ static void etm4_init_arch_data(void *info)
> >  	etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
> >  
> >  	/* INSTP0, bits[2:1] P0 tracing support field */
> > -	if (BMVAL(etmidr0, 1, 2) == 0b11)
> > -		drvdata->instrp0 = true;
> > -	else
> > -		drvdata->instrp0 = false;
> > -
> > +	drvdata->instrp0 = !!(REG_VAL(etmidr0, TRCIDR0_INSTP0) == 0b11);
> >  	/* TRCBB, bit[5] Branch broadcast tracing support bit */
> > -	if (BMVAL(etmidr0, 5, 5))
> > -		drvdata->trcbb = true;
> > -	else
> > -		drvdata->trcbb = false;
> > -
> > +	drvdata->trcbb = !!(etmidr0 & TRCIDR0_TRCBB);
> >  	/* TRCCOND, bit[6] Conditional instruction tracing support bit */
> > -	if (BMVAL(etmidr0, 6, 6))
> > -		drvdata->trccond = true;
> > -	else
> > -		drvdata->trccond = false;
> > -
> > +	drvdata->trccond = !!(etmidr0 & TRCIDR0_TRCCOND);
> >  	/* TRCCCI, bit[7] Cycle counting instruction bit */
> > -	if (BMVAL(etmidr0, 7, 7))
> > -		drvdata->trccci = true;
> > -	else
> > -		drvdata->trccci = false;
> > -
> > +	drvdata->trccci = !!(etmidr0 & TRCIDR0_TRCCCI);
> >  	/* RETSTACK, bit[9] Return stack bit */
> > -	if (BMVAL(etmidr0, 9, 9))
> > -		drvdata->retstack = true;
> > -	else
> > -		drvdata->retstack = false;
> > -
> > +	drvdata->retstack = !!(etmidr0 & TRCIDR0_RETSTACK);
> >  	/* NUMEVENT, bits[11:10] Number of events field */
> > -	drvdata->nr_event = BMVAL(etmidr0, 10, 11);
> > +	drvdata->nr_event = REG_VAL(etmidr0, TRCIDR0_NUMEVENT);
> >  	/* QSUPP, bits[16:15] Q element support field */
> > -	drvdata->q_support = BMVAL(etmidr0, 15, 16);
> > +	drvdata->q_support = REG_VAL(etmidr0, TRCIDR0_QSUPP);
> >  	/* TSSIZE, bits[28:24] Global timestamp size field */
> > -	drvdata->ts_size = BMVAL(etmidr0, 24, 28);
> > +	drvdata->ts_size = REG_VAL(etmidr0, TRCIDR0_TSSIZE);
> >  
> >  	/* maximum size of resources */
> >  	etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> > index 3c4d69b096ca..2bd8ad953b8e 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> > @@ -130,6 +130,23 @@
> >  
> >  #define TRCRSR_TA			BIT(12)
> >  
> > +/*
> > + * Bit positions of registers that are defined above, in the sysreg.h style
> > + * of _MASK, _SHIFT and BIT().
> > + */
> 
> ^^^ not really necessary. Instead the format requirement for below mentioned
> CORESIGHT_REG_VAL() macro might be relevant and should be mentioned.

I'm not sure if you're asking to move the comment further below or remove it
altogether.  In any case more comments is always better than less.

> 
> > +#define TRCIDR0_INSTP0_SHIFT			1
> > +#define TRCIDR0_INSTP0_MASK			GENMASK(1, 0)
> > +#define TRCIDR0_TRCBB				BIT(5)
> > +#define TRCIDR0_TRCCOND				BIT(6)
> > +#define TRCIDR0_TRCCCI				BIT(7)
> > +#define TRCIDR0_RETSTACK			BIT(9)
> > +#define TRCIDR0_NUMEVENT_SHIFT			10
> > +#define TRCIDR0_NUMEVENT_MASK			GENMASK(1, 0)
> > +#define TRCIDR0_QSUPP_SHIFT			15
> > +#define TRCIDR0_QSUPP_MASK			GENMASK(1, 0)
> > +#define TRCIDR0_TSSIZE_SHIFT			24
> > +#define TRCIDR0_TSSIZE_MASK			GENMASK(4, 0)
> > +
> >  /*
> >   * System instructions to access ETM registers.
> >   * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> > index ff1dd2092ac5..1452c6038421 100644
> > --- a/drivers/hwtracing/coresight/coresight-priv.h
> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> > @@ -36,6 +36,11 @@
> >  
> >  #define TIMEOUT_US		100
> >  #define BMVAL(val, lsb, msb)	((val & GENMASK(msb, lsb)) >> lsb)
> > +/*
> > + * Extract a field from a register where field is #defined in the form
> > + * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
> > + */
> 
> Looking at the usage, <register_name> is already embedded in <filed_name>. So
> it requires <field_name>_SHIFT and <field_name>_MASK instead. Unless register
> name should be passed as separate argument (which actually might be better).
> 
> REG_VAL(etmidr0, TRCIDR0_TSSIZE) ----> REG_VAL(etmidr0, TRCIDR0, TSSIZE)

I don't have strong preference, I'm good either way.

> 
> with some restructuring in the comment ..
> 
> /*
>  * Extract a field from a coresight register
>  *
>  * Required fields are defined as macros like the following
>  *  
>  * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
>  */
> 
> > +#define REG_VAL(val, field)	((val & (field##_MASK << field##_SHIFT)) >> field##_SHIFT)
> 
> This is too generic to be in a coresight header or it should just be
> named CORESIGHT_REG_VAL() instead, making it more specific for here.
> 
> The build should fail in case any required macro definition is absent.
> I guess no more fortification is required in case macros are missing.
> 
> However CORESIGHT_REG_VAL() is better placed in <coresight-etm4x.h>
> just before all the dependent SHIFT/MASK register field definition
> starts.
> 

Not sure about this...  Someone might want to do the same for etmv3 and in that
case we'll end up moving the macro again.

Thanks,
Mathieu

> >  
> >  #define ETM_MODE_EXCL_KERN	BIT(30)
> >  #define ETM_MODE_EXCL_USER	BIT(31)
> > 
> 
> - Anshuman

Re: [PATCH v2 07/15] coresight: Make ETM4x TRCEVENTCTL1R register accesses consistent with sysreg.h

[Mathieu Poirier]

On Thu, Feb 03, 2022 at 12:05:55PM +0000, James Clark wrote:
> This is a no-op change for style and consistency and has no effect on the
> binary produced by gcc-11.
> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  .../coresight/coresight-etm4x-sysfs.c         | 25 +++++++++++--------
>  drivers/hwtracing/coresight/coresight-etm4x.h |  9 +++++++
>  2 files changed, 24 insertions(+), 10 deletions(-)
>

I like what this patchset is doing.  I have reviewed up to here today and will
finish tomorrow.

Thanks,
Mathieu

> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> index 4c29ab4464a0..cfa6f72a1e39 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> @@ -384,16 +384,16 @@ static ssize_t mode_store(struct device *dev,
>  	/* bit[11], AMBA Trace Bus (ATB) trigger enable bit */
>  	if ((config->mode & ETM_MODE_ATB_TRIGGER) &&
>  	    (drvdata->atbtrig == true))
> -		config->eventctrl1 |= BIT(11);
> +		config->eventctrl1 |= TRCEVENTCTL1R_ATB;
>  	else
> -		config->eventctrl1 &= ~BIT(11);
> +		config->eventctrl1 &= ~TRCEVENTCTL1R_ATB;
>  
>  	/* bit[12], Low-power state behavior override bit */
>  	if ((config->mode & ETM_MODE_LPOVERRIDE) &&
>  	    (drvdata->lpoverride == true))
> -		config->eventctrl1 |= BIT(12);
> +		config->eventctrl1 |= TRCEVENTCTL1R_LPOVERRIDE;
>  	else
> -		config->eventctrl1 &= ~BIT(12);
> +		config->eventctrl1 &= ~TRCEVENTCTL1R_LPOVERRIDE;
>  
>  	/* bit[8], Instruction stall bit */
>  	if ((config->mode & ETM_MODE_ISTALL_EN) && (drvdata->stallctl == true))
> @@ -534,7 +534,7 @@ static ssize_t event_instren_show(struct device *dev,
>  	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
>  	struct etmv4_config *config = &drvdata->config;
>  
> -	val = BMVAL(config->eventctrl1, 0, 3);
> +	val = REG_VAL(config->eventctrl1, TRCEVENTCTL1R_INSTEN);
>  	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>  }
>  
> @@ -551,23 +551,28 @@ static ssize_t event_instren_store(struct device *dev,
>  
>  	spin_lock(&drvdata->spinlock);
>  	/* start by clearing all instruction event enable bits */
> -	config->eventctrl1 &= ~(BIT(0) | BIT(1) | BIT(2) | BIT(3));
> +	config->eventctrl1 &= ~(TRCEVENTCTL1R_INSTEN_MASK << TRCEVENTCTL1R_INSTEN_SHIFT);
>  	switch (drvdata->nr_event) {
>  	case 0x0:
>  		/* generate Event element for event 1 */
> -		config->eventctrl1 |= val & BIT(1);
> +		config->eventctrl1 |= val & TRCEVENTCTL1R_INSTEN_1;
>  		break;
>  	case 0x1:
>  		/* generate Event element for event 1 and 2 */
> -		config->eventctrl1 |= val & (BIT(0) | BIT(1));
> +		config->eventctrl1 |= val & (TRCEVENTCTL1R_INSTEN_0 | TRCEVENTCTL1R_INSTEN_1);
>  		break;
>  	case 0x2:
>  		/* generate Event element for event 1, 2 and 3 */
> -		config->eventctrl1 |= val & (BIT(0) | BIT(1) | BIT(2));
> +		config->eventctrl1 |= val & (TRCEVENTCTL1R_INSTEN_0 |
> +					     TRCEVENTCTL1R_INSTEN_1 |
> +					     TRCEVENTCTL1R_INSTEN_2);
>  		break;
>  	case 0x3:
>  		/* generate Event element for all 4 events */
> -		config->eventctrl1 |= val & 0xF;
> +		config->eventctrl1 |= val & (TRCEVENTCTL1R_INSTEN_0 |
> +					     TRCEVENTCTL1R_INSTEN_1 |
> +					     TRCEVENTCTL1R_INSTEN_2 |
> +					     TRCEVENTCTL1R_INSTEN_3);
>  		break;
>  	default:
>  		break;
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 55e756020a94..eb72b81bbf85 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -211,6 +211,15 @@
>  #define TRCCONFIGR_DA				BIT(16)
>  #define TRCCONFIGR_DV				BIT(17)
>  
> +#define TRCEVENTCTL1R_INSTEN_SHIFT		0
> +#define TRCEVENTCTL1R_INSTEN_MASK		GENMASK(3, 0)
> +#define TRCEVENTCTL1R_INSTEN_0			BIT(0)
> +#define TRCEVENTCTL1R_INSTEN_1			BIT(1)
> +#define TRCEVENTCTL1R_INSTEN_2			BIT(2)
> +#define TRCEVENTCTL1R_INSTEN_3			BIT(3)
> +#define TRCEVENTCTL1R_ATB			BIT(11)
> +#define TRCEVENTCTL1R_LPOVERRIDE		BIT(12)
> +
>  /*
>   * System instructions to access ETM registers.
>   * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
> -- 
> 2.28.0
> 

Re: [PATCH v2 01/15] coresight: Make ETM4x TRCIDR0 register accesses consistent with sysreg.h

[Mike Leach]

Hi James,

On Thu, 3 Feb 2022 at 12:06, James Clark <james.clark@arm.com> wrote:
>
> This is a no-op change for style and consistency and has no effect on the
> binary produced by gcc-11.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  .../coresight/coresight-etm4x-core.c          | 36 +++++--------------
>  drivers/hwtracing/coresight/coresight-etm4x.h | 17 +++++++++
>  drivers/hwtracing/coresight/coresight-priv.h  |  5 +++
>  3 files changed, 30 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index e2eebd865241..107e81948f76 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1091,41 +1091,21 @@ static void etm4_init_arch_data(void *info)
>         etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
>
>         /* INSTP0, bits[2:1] P0 tracing support field */
> -       if (BMVAL(etmidr0, 1, 2) == 0b11)
> -               drvdata->instrp0 = true;
> -       else
> -               drvdata->instrp0 = false;
> -
> +       drvdata->instrp0 = !!(REG_VAL(etmidr0, TRCIDR0_INSTP0) == 0b11);
>         /* TRCBB, bit[5] Branch broadcast tracing support bit */
> -       if (BMVAL(etmidr0, 5, 5))
> -               drvdata->trcbb = true;
> -       else
> -               drvdata->trcbb = false;
> -
> +       drvdata->trcbb = !!(etmidr0 & TRCIDR0_TRCBB);
>         /* TRCCOND, bit[6] Conditional instruction tracing support bit */
> -       if (BMVAL(etmidr0, 6, 6))
> -               drvdata->trccond = true;
> -       else
> -               drvdata->trccond = false;
> -
> +       drvdata->trccond = !!(etmidr0 & TRCIDR0_TRCCOND);
>         /* TRCCCI, bit[7] Cycle counting instruction bit */
> -       if (BMVAL(etmidr0, 7, 7))
> -               drvdata->trccci = true;
> -       else
> -               drvdata->trccci = false;
> -
> +       drvdata->trccci = !!(etmidr0 & TRCIDR0_TRCCCI);
>         /* RETSTACK, bit[9] Return stack bit */
> -       if (BMVAL(etmidr0, 9, 9))
> -               drvdata->retstack = true;
> -       else
> -               drvdata->retstack = false;
> -
> +       drvdata->retstack = !!(etmidr0 & TRCIDR0_RETSTACK);
>         /* NUMEVENT, bits[11:10] Number of events field */
> -       drvdata->nr_event = BMVAL(etmidr0, 10, 11);
> +       drvdata->nr_event = REG_VAL(etmidr0, TRCIDR0_NUMEVENT);
>         /* QSUPP, bits[16:15] Q element support field */
> -       drvdata->q_support = BMVAL(etmidr0, 15, 16);
> +       drvdata->q_support = REG_VAL(etmidr0, TRCIDR0_QSUPP);
>         /* TSSIZE, bits[28:24] Global timestamp size field */
> -       drvdata->ts_size = BMVAL(etmidr0, 24, 28);
> +       drvdata->ts_size = REG_VAL(etmidr0, TRCIDR0_TSSIZE);
>
>         /* maximum size of resources */
>         etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 3c4d69b096ca..2bd8ad953b8e 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -130,6 +130,23 @@
>
>  #define TRCRSR_TA                      BIT(12)
>
> +/*
> + * Bit positions of registers that are defined above, in the sysreg.h style
> + * of _MASK, _SHIFT and BIT().
> + */
> +#define TRCIDR0_INSTP0_SHIFT                   1
> +#define TRCIDR0_INSTP0_MASK                    GENMASK(1, 0)
> +#define TRCIDR0_TRCBB                          BIT(5)
> +#define TRCIDR0_TRCCOND                                BIT(6)
> +#define TRCIDR0_TRCCCI                         BIT(7)
> +#define TRCIDR0_RETSTACK                       BIT(9)
> +#define TRCIDR0_NUMEVENT_SHIFT                 10
> +#define TRCIDR0_NUMEVENT_MASK                  GENMASK(1, 0)
> +#define TRCIDR0_QSUPP_SHIFT                    15
> +#define TRCIDR0_QSUPP_MASK                     GENMASK(1, 0)
> +#define TRCIDR0_TSSIZE_SHIFT                   24
> +#define TRCIDR0_TSSIZE_MASK                    GENMASK(4, 0)
> +
>  /*
>   * System instructions to access ETM registers.
>   * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index ff1dd2092ac5..1452c6038421 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -36,6 +36,11 @@
>
>  #define TIMEOUT_US             100
>  #define BMVAL(val, lsb, msb)   ((val & GENMASK(msb, lsb)) >> lsb)
> +/*
> + * Extract a field from a register where field is #defined in the form
> + * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
> + */
> +#define REG_VAL(val, field)    ((val & (field##_MASK << field##_SHIFT)) >> field##_SHIFT)
>

The only v minor comment I would have is that this macro could be
REG_FIELD() or REG_FIELD_VAL(), which in my view would make more
natural and obvious reading in the source files where it appears.

e.g.

drvdata->nr_event = REG_FIELD(etmidr0, TRCIDR0_NUMEVENT);

However it ends up, it certainly needs to stay in a common header for
possible use in other drivers (not just ETMv3).

Regards

Mike

>  #define ETM_MODE_EXCL_KERN     BIT(30)
>  #define ETM_MODE_EXCL_USER     BIT(31)
> --
> 2.28.0
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH v2 01/15] coresight: Make ETM4x TRCIDR0 register accesses consistent with sysreg.h

[Suzuki K Poulose]

On 07/02/2022 05:44, Anshuman Khandual wrote:
> Hi James,
> 
> These are all ETM4X specific changes. Something like this might be cleaner
> and also more compact. Also would suggest to follow the same for subsequent
> patches as well.
> 
> coresight: etm4x: Cleanup TRCIDR0 register accesses
> 
> Consistency with sysreg.h could be mentioned in the commit message itself.
> 
> On 2/3/22 5:35 PM, James Clark wrote:
>> This is a no-op change for style and consistency and has no effect on the
>> binary produced by gcc-11.
> 
> This patch adds register definitions, helper macros as well. Please expand
> the commit message to add more details. This is too short, for the change
> it creates. BTW why is it necessary to mention GCC version number here.
> 
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>   .../coresight/coresight-etm4x-core.c          | 36 +++++--------------
>>   drivers/hwtracing/coresight/coresight-etm4x.h | 17 +++++++++
>>   drivers/hwtracing/coresight/coresight-priv.h  |  5 +++
>>   3 files changed, 30 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index e2eebd865241..107e81948f76 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -1091,41 +1091,21 @@ static void etm4_init_arch_data(void *info)
>>   	etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
>>   
>>   	/* INSTP0, bits[2:1] P0 tracing support field */
>> -	if (BMVAL(etmidr0, 1, 2) == 0b11)
>> -		drvdata->instrp0 = true;
>> -	else
>> -		drvdata->instrp0 = false;
>> -
>> +	drvdata->instrp0 = !!(REG_VAL(etmidr0, TRCIDR0_INSTP0) == 0b11);
>>   	/* TRCBB, bit[5] Branch broadcast tracing support bit */
>> -	if (BMVAL(etmidr0, 5, 5))
>> -		drvdata->trcbb = true;
>> -	else
>> -		drvdata->trcbb = false;
>> -
>> +	drvdata->trcbb = !!(etmidr0 & TRCIDR0_TRCBB);
>>   	/* TRCCOND, bit[6] Conditional instruction tracing support bit */
>> -	if (BMVAL(etmidr0, 6, 6))
>> -		drvdata->trccond = true;
>> -	else
>> -		drvdata->trccond = false;
>> -
>> +	drvdata->trccond = !!(etmidr0 & TRCIDR0_TRCCOND);
>>   	/* TRCCCI, bit[7] Cycle counting instruction bit */
>> -	if (BMVAL(etmidr0, 7, 7))
>> -		drvdata->trccci = true;
>> -	else
>> -		drvdata->trccci = false;
>> -
>> +	drvdata->trccci = !!(etmidr0 & TRCIDR0_TRCCCI);
>>   	/* RETSTACK, bit[9] Return stack bit */
>> -	if (BMVAL(etmidr0, 9, 9))
>> -		drvdata->retstack = true;
>> -	else
>> -		drvdata->retstack = false;
>> -
>> +	drvdata->retstack = !!(etmidr0 & TRCIDR0_RETSTACK);
>>   	/* NUMEVENT, bits[11:10] Number of events field */
>> -	drvdata->nr_event = BMVAL(etmidr0, 10, 11);
>> +	drvdata->nr_event = REG_VAL(etmidr0, TRCIDR0_NUMEVENT);
>>   	/* QSUPP, bits[16:15] Q element support field */
>> -	drvdata->q_support = BMVAL(etmidr0, 15, 16);
>> +	drvdata->q_support = REG_VAL(etmidr0, TRCIDR0_QSUPP);
>>   	/* TSSIZE, bits[28:24] Global timestamp size field */
>> -	drvdata->ts_size = BMVAL(etmidr0, 24, 28);
>> +	drvdata->ts_size = REG_VAL(etmidr0, TRCIDR0_TSSIZE);
>>   
>>   	/* maximum size of resources */
>>   	etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>> index 3c4d69b096ca..2bd8ad953b8e 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>> @@ -130,6 +130,23 @@
>>   
>>   #define TRCRSR_TA			BIT(12)
>>   
>> +/*
>> + * Bit positions of registers that are defined above, in the sysreg.h style
>> + * of _MASK, _SHIFT and BIT().
>> + */
> 
> ^^^ not really necessary. Instead the format requirement for below mentioned
> CORESIGHT_REG_VAL() macro might be relevant and should be mentioned.
> 
>> +#define TRCIDR0_INSTP0_SHIFT			1
>> +#define TRCIDR0_INSTP0_MASK			GENMASK(1, 0)
>> +#define TRCIDR0_TRCBB				BIT(5)
>> +#define TRCIDR0_TRCCOND				BIT(6)
>> +#define TRCIDR0_TRCCCI				BIT(7)
>> +#define TRCIDR0_RETSTACK			BIT(9)
>> +#define TRCIDR0_NUMEVENT_SHIFT			10
>> +#define TRCIDR0_NUMEVENT_MASK			GENMASK(1, 0)
>> +#define TRCIDR0_QSUPP_SHIFT			15
>> +#define TRCIDR0_QSUPP_MASK			GENMASK(1, 0)
>> +#define TRCIDR0_TSSIZE_SHIFT			24
>> +#define TRCIDR0_TSSIZE_MASK			GENMASK(4, 0)
>> +
>>   /*
>>    * System instructions to access ETM registers.
>>    * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
>> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
>> index ff1dd2092ac5..1452c6038421 100644
>> --- a/drivers/hwtracing/coresight/coresight-priv.h
>> +++ b/drivers/hwtracing/coresight/coresight-priv.h
>> @@ -36,6 +36,11 @@
>>   
>>   #define TIMEOUT_US		100
>>   #define BMVAL(val, lsb, msb)	((val & GENMASK(msb, lsb)) >> lsb)
>> +/*
>> + * Extract a field from a register where field is #defined in the form
>> + * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
>> + */
> 
> Looking at the usage, <register_name> is already embedded in <filed_name>. So
> it requires <field_name>_SHIFT and <field_name>_MASK instead. Unless register
> name should be passed as separate argument (which actually might be better).
> 
> REG_VAL(etmidr0, TRCIDR0_TSSIZE) ----> REG_VAL(etmidr0, TRCIDR0, TSSIZE)

I don't see much difference here. So I am fine either way.

> 
> with some restructuring in the comment ..
> 
> /*
>   * Extract a field from a coresight register
>   *
>   * Required fields are defined as macros like the following
>   *
>   * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
>   */
> 
>> +#define REG_VAL(val, field)	((val & (field##_MASK << field##_SHIFT)) >> field##_SHIFT)
> 
> This is too generic to be in a coresight header or it should just be
> named CORESIGHT_REG_VAL() instead, making it more specific for here.
> 
> The build should fail in case any required macro definition is absent.
> I guess no more fortification is required in case macros are missing.
> 
> However CORESIGHT_REG_VAL() is better placed in <coresight-etm4x.h>
> just before all the dependent SHIFT/MASK register field definition
> starts.

Not necessarily. CORESIGHT_REG_VAL() is a generic function and doesn't
have anything specific to do with etm4x. We could reuse that for
cleaning up other drivers in CoreSight.

Cheers
Suzuki

Re: [PATCH v2 15/15] coresight: Make ETM4x TRCRSCTLRn register accesses consistent with sysreg.h

[Mathieu Poirier]

On Thu, Feb 03, 2022 at 12:06:03PM +0000, James Clark wrote:
> This is a no-op change for style and consistency and has no effect on the
> binary produced by gcc-11.
> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 7 +++++--
>  drivers/hwtracing/coresight/coresight-etm4x.h       | 9 +++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> index a0cdd2cd978a..c876a63fa84d 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> @@ -1728,8 +1728,11 @@ static ssize_t res_ctrl_store(struct device *dev,
>  	/* For odd idx pair inversal bit is RES0 */
>  	if (idx % 2 != 0)
>  		/* PAIRINV, bit[21] */
> -		val &= ~BIT(21);
> -	config->res_ctrl[idx] = val & GENMASK(21, 0);
> +		val &= ~TRCRSCTLRn_PAIRINV;
> +	config->res_ctrl[idx] = val & (TRCRSCTLRn_PAIRINV |
> +				       TRCRSCTLRn_INV |
> +				       (TRCRSCTLRn_GROUP_MASK << TRCRSCTLRn_GROUP_SHIFT) |
> +				       (TRCRSCTLRn_SELECT_MASK << TRCRSCTLRn_SELECT_SHIFT));
>  	spin_unlock(&drvdata->spinlock);
>  	return size;
>  }
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 4d943faade33..dd2156a5e70b 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -258,6 +258,15 @@
>  #define TRCBBCTLR_RANGE_SHIFT			0
>  #define TRCBBCTLR_RANGE_MASK			GENMASK(7, 0)
>  
> +#define TRCRSCTLRn_PAIRINV			BIT(21)
> +#define TRCRSCTLRn_INV				BIT(20)
> +#define TRCRSCTLRn_GROUP_SHIFT			16
> +#define TRCRSCTLRn_GROUP_MASK			GENMASK(3, 0)
> +#define TRCRSCTLRn_SELECT_SHIFT			0
> +#define TRCRSCTLRn_SELECT_MASK			GENMASK(15, 0)
> +
> +
> +

Two extra newlines.

With the above and for patches 02 to 15:

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

>  /*
>   * System instructions to access ETM registers.
>   * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
> -- 
> 2.28.0
> 

Re: [PATCH v2 01/15] coresight: Make ETM4x TRCIDR0 register accesses consistent with sysreg.h

[Mike Leach]

Hi James,

When reviewing another patch set I spotted that there is a FIELD_GET
macro already defined for the kernel in include/linux/bitfield.h

Advantage of this is that you only need to define a shifted mask for
each field and the macro handles the extraction and shifting
automatically.
It also ensures does a bunch of compile time sanity checks to ensure
that the masks are in range for the type.

i.e. - define the mask _with_ the shift
#define TRCIDR0_INSTP0_MASK                    GENMASK(2, 1)

then
drvdata->instrp0 = !!(FIELD_GET(TRCIDR0_INSTP0_MASK, etmidr0) == 0b11);

which means all the shift #defines can be dropped

Should we use this and drop the REG_VAL or whatever?

Regards

Mike

On Tue, 8 Feb 2022 at 15:04, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 07/02/2022 05:44, Anshuman Khandual wrote:
> > Hi James,
> >
> > These are all ETM4X specific changes. Something like this might be cleaner
> > and also more compact. Also would suggest to follow the same for subsequent
> > patches as well.
> >
> > coresight: etm4x: Cleanup TRCIDR0 register accesses
> >
> > Consistency with sysreg.h could be mentioned in the commit message itself.
> >
> > On 2/3/22 5:35 PM, James Clark wrote:
> >> This is a no-op change for style and consistency and has no effect on the
> >> binary produced by gcc-11.
> >
> > This patch adds register definitions, helper macros as well. Please expand
> > the commit message to add more details. This is too short, for the change
> > it creates. BTW why is it necessary to mention GCC version number here.
> >
> >>
> >> Signed-off-by: James Clark <james.clark@arm.com>
> >> ---
> >>   .../coresight/coresight-etm4x-core.c          | 36 +++++--------------
> >>   drivers/hwtracing/coresight/coresight-etm4x.h | 17 +++++++++
> >>   drivers/hwtracing/coresight/coresight-priv.h  |  5 +++
> >>   3 files changed, 30 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >> index e2eebd865241..107e81948f76 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >> @@ -1091,41 +1091,21 @@ static void etm4_init_arch_data(void *info)
> >>      etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
> >>
> >>      /* INSTP0, bits[2:1] P0 tracing support field */
> >> -    if (BMVAL(etmidr0, 1, 2) == 0b11)
> >> -            drvdata->instrp0 = true;
> >> -    else
> >> -            drvdata->instrp0 = false;
> >> -
> >> +    drvdata->instrp0 = !!(REG_VAL(etmidr0, TRCIDR0_INSTP0) == 0b11);
> >>      /* TRCBB, bit[5] Branch broadcast tracing support bit */
> >> -    if (BMVAL(etmidr0, 5, 5))
> >> -            drvdata->trcbb = true;
> >> -    else
> >> -            drvdata->trcbb = false;
> >> -
> >> +    drvdata->trcbb = !!(etmidr0 & TRCIDR0_TRCBB);
> >>      /* TRCCOND, bit[6] Conditional instruction tracing support bit */
> >> -    if (BMVAL(etmidr0, 6, 6))
> >> -            drvdata->trccond = true;
> >> -    else
> >> -            drvdata->trccond = false;
> >> -
> >> +    drvdata->trccond = !!(etmidr0 & TRCIDR0_TRCCOND);
> >>      /* TRCCCI, bit[7] Cycle counting instruction bit */
> >> -    if (BMVAL(etmidr0, 7, 7))
> >> -            drvdata->trccci = true;
> >> -    else
> >> -            drvdata->trccci = false;
> >> -
> >> +    drvdata->trccci = !!(etmidr0 & TRCIDR0_TRCCCI);
> >>      /* RETSTACK, bit[9] Return stack bit */
> >> -    if (BMVAL(etmidr0, 9, 9))
> >> -            drvdata->retstack = true;
> >> -    else
> >> -            drvdata->retstack = false;
> >> -
> >> +    drvdata->retstack = !!(etmidr0 & TRCIDR0_RETSTACK);
> >>      /* NUMEVENT, bits[11:10] Number of events field */
> >> -    drvdata->nr_event = BMVAL(etmidr0, 10, 11);
> >> +    drvdata->nr_event = REG_VAL(etmidr0, TRCIDR0_NUMEVENT);
> >>      /* QSUPP, bits[16:15] Q element support field */
> >> -    drvdata->q_support = BMVAL(etmidr0, 15, 16);
> >> +    drvdata->q_support = REG_VAL(etmidr0, TRCIDR0_QSUPP);
> >>      /* TSSIZE, bits[28:24] Global timestamp size field */
> >> -    drvdata->ts_size = BMVAL(etmidr0, 24, 28);
> >> +    drvdata->ts_size = REG_VAL(etmidr0, TRCIDR0_TSSIZE);
> >>
> >>      /* maximum size of resources */
> >>      etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> >> index 3c4d69b096ca..2bd8ad953b8e 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> >> @@ -130,6 +130,23 @@
> >>
> >>   #define TRCRSR_TA                  BIT(12)
> >>
> >> +/*
> >> + * Bit positions of registers that are defined above, in the sysreg.h style
> >> + * of _MASK, _SHIFT and BIT().
> >> + */
> >
> > ^^^ not really necessary. Instead the format requirement for below mentioned
> > CORESIGHT_REG_VAL() macro might be relevant and should be mentioned.
> >
> >> +#define TRCIDR0_INSTP0_SHIFT                        1
> >> +#define TRCIDR0_INSTP0_MASK                 GENMASK(1, 0)
> >> +#define TRCIDR0_TRCBB                               BIT(5)
> >> +#define TRCIDR0_TRCCOND                             BIT(6)
> >> +#define TRCIDR0_TRCCCI                              BIT(7)
> >> +#define TRCIDR0_RETSTACK                    BIT(9)
> >> +#define TRCIDR0_NUMEVENT_SHIFT                      10
> >> +#define TRCIDR0_NUMEVENT_MASK                       GENMASK(1, 0)
> >> +#define TRCIDR0_QSUPP_SHIFT                 15
> >> +#define TRCIDR0_QSUPP_MASK                  GENMASK(1, 0)
> >> +#define TRCIDR0_TSSIZE_SHIFT                        24
> >> +#define TRCIDR0_TSSIZE_MASK                 GENMASK(4, 0)
> >> +
> >>   /*
> >>    * System instructions to access ETM registers.
> >>    * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
> >> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> >> index ff1dd2092ac5..1452c6038421 100644
> >> --- a/drivers/hwtracing/coresight/coresight-priv.h
> >> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> >> @@ -36,6 +36,11 @@
> >>
> >>   #define TIMEOUT_US         100
> >>   #define BMVAL(val, lsb, msb)       ((val & GENMASK(msb, lsb)) >> lsb)
> >> +/*
> >> + * Extract a field from a register where field is #defined in the form
> >> + * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
> >> + */
> >
> > Looking at the usage, <register_name> is already embedded in <filed_name>. So
> > it requires <field_name>_SHIFT and <field_name>_MASK instead. Unless register
> > name should be passed as separate argument (which actually might be better).
> >
> > REG_VAL(etmidr0, TRCIDR0_TSSIZE) ----> REG_VAL(etmidr0, TRCIDR0, TSSIZE)
>
> I don't see much difference here. So I am fine either way.
>
> >
> > with some restructuring in the comment ..
> >
> > /*
> >   * Extract a field from a coresight register
> >   *
> >   * Required fields are defined as macros like the following
> >   *
> >   * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
> >   */
> >
> >> +#define REG_VAL(val, field) ((val & (field##_MASK << field##_SHIFT)) >> field##_SHIFT)
> >
> > This is too generic to be in a coresight header or it should just be
> > named CORESIGHT_REG_VAL() instead, making it more specific for here.
> >
> > The build should fail in case any required macro definition is absent.
> > I guess no more fortification is required in case macros are missing.
> >
> > However CORESIGHT_REG_VAL() is better placed in <coresight-etm4x.h>
> > just before all the dependent SHIFT/MASK register field definition
> > starts.
>
> Not necessarily. CORESIGHT_REG_VAL() is a generic function and doesn't
> have anything specific to do with etm4x. We could reuse that for
> cleaning up other drivers in CoreSight.
>
> Cheers
> Suzuki



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH v2 01/15] coresight: Make ETM4x TRCIDR0 register accesses consistent with sysreg.h

[James Clark]

On 09/02/2022 09:32, Mike Leach wrote:
> Hi James,
> 
> When reviewing another patch set I spotted that there is a FIELD_GET
> macro already defined for the kernel in include/linux/bitfield.h
> 
> Advantage of this is that you only need to define a shifted mask for
> each field and the macro handles the extraction and shifting
> automatically.
> It also ensures does a bunch of compile time sanity checks to ensure
> that the masks are in range for the type.
> 
> i.e. - define the mask _with_ the shift
> #define TRCIDR0_INSTP0_MASK                    GENMASK(2, 1)
> 
> then
> drvdata->instrp0 = !!(FIELD_GET(TRCIDR0_INSTP0_MASK, etmidr0) == 0b11);
> 
> which means all the shift #defines can be dropped
> 
> Should we use this and drop the REG_VAL or whatever?

To me it seems like this is a much better way of doing it because it removes
one source of error with the shift being incorrect. This way it also mirrors
the reference manual more closely.

One possible issue is that it's not really consistent with sysreg.h because
masks there are from the 0 bit and the shift is used. I suppose doing it this
way is still closer to sysreg.h so it's a net benefit.

I checked converting one register to this format and the compiler still emits
the same thing so if we want to we can go ahead and do all of them.
I'm leaning towards this way because this is what I would have done if it was
from scratch. I just did it with the mask and the shift separately for
consistency.


> 
> Regards
> 
> Mike
> 
> On Tue, 8 Feb 2022 at 15:04, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> On 07/02/2022 05:44, Anshuman Khandual wrote:
>>> Hi James,
>>>
>>> These are all ETM4X specific changes. Something like this might be cleaner
>>> and also more compact. Also would suggest to follow the same for subsequent
>>> patches as well.
>>>
>>> coresight: etm4x: Cleanup TRCIDR0 register accesses
>>>
>>> Consistency with sysreg.h could be mentioned in the commit message itself.
>>>
>>> On 2/3/22 5:35 PM, James Clark wrote:
>>>> This is a no-op change for style and consistency and has no effect on the
>>>> binary produced by gcc-11.
>>>
>>> This patch adds register definitions, helper macros as well. Please expand
>>> the commit message to add more details. This is too short, for the change
>>> it creates. BTW why is it necessary to mention GCC version number here.
>>>
>>>>
>>>> Signed-off-by: James Clark <james.clark@arm.com>
>>>> ---
>>>>   .../coresight/coresight-etm4x-core.c          | 36 +++++--------------
>>>>   drivers/hwtracing/coresight/coresight-etm4x.h | 17 +++++++++
>>>>   drivers/hwtracing/coresight/coresight-priv.h  |  5 +++
>>>>   3 files changed, 30 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> index e2eebd865241..107e81948f76 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> @@ -1091,41 +1091,21 @@ static void etm4_init_arch_data(void *info)
>>>>      etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
>>>>
>>>>      /* INSTP0, bits[2:1] P0 tracing support field */
>>>> -    if (BMVAL(etmidr0, 1, 2) == 0b11)
>>>> -            drvdata->instrp0 = true;
>>>> -    else
>>>> -            drvdata->instrp0 = false;
>>>> -
>>>> +    drvdata->instrp0 = !!(REG_VAL(etmidr0, TRCIDR0_INSTP0) == 0b11);
>>>>      /* TRCBB, bit[5] Branch broadcast tracing support bit */
>>>> -    if (BMVAL(etmidr0, 5, 5))
>>>> -            drvdata->trcbb = true;
>>>> -    else
>>>> -            drvdata->trcbb = false;
>>>> -
>>>> +    drvdata->trcbb = !!(etmidr0 & TRCIDR0_TRCBB);
>>>>      /* TRCCOND, bit[6] Conditional instruction tracing support bit */
>>>> -    if (BMVAL(etmidr0, 6, 6))
>>>> -            drvdata->trccond = true;
>>>> -    else
>>>> -            drvdata->trccond = false;
>>>> -
>>>> +    drvdata->trccond = !!(etmidr0 & TRCIDR0_TRCCOND);
>>>>      /* TRCCCI, bit[7] Cycle counting instruction bit */
>>>> -    if (BMVAL(etmidr0, 7, 7))
>>>> -            drvdata->trccci = true;
>>>> -    else
>>>> -            drvdata->trccci = false;
>>>> -
>>>> +    drvdata->trccci = !!(etmidr0 & TRCIDR0_TRCCCI);
>>>>      /* RETSTACK, bit[9] Return stack bit */
>>>> -    if (BMVAL(etmidr0, 9, 9))
>>>> -            drvdata->retstack = true;
>>>> -    else
>>>> -            drvdata->retstack = false;
>>>> -
>>>> +    drvdata->retstack = !!(etmidr0 & TRCIDR0_RETSTACK);
>>>>      /* NUMEVENT, bits[11:10] Number of events field */
>>>> -    drvdata->nr_event = BMVAL(etmidr0, 10, 11);
>>>> +    drvdata->nr_event = REG_VAL(etmidr0, TRCIDR0_NUMEVENT);
>>>>      /* QSUPP, bits[16:15] Q element support field */
>>>> -    drvdata->q_support = BMVAL(etmidr0, 15, 16);
>>>> +    drvdata->q_support = REG_VAL(etmidr0, TRCIDR0_QSUPP);
>>>>      /* TSSIZE, bits[28:24] Global timestamp size field */
>>>> -    drvdata->ts_size = BMVAL(etmidr0, 24, 28);
>>>> +    drvdata->ts_size = REG_VAL(etmidr0, TRCIDR0_TSSIZE);
>>>>
>>>>      /* maximum size of resources */
>>>>      etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>>>> index 3c4d69b096ca..2bd8ad953b8e 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>>>> @@ -130,6 +130,23 @@
>>>>
>>>>   #define TRCRSR_TA                  BIT(12)
>>>>
>>>> +/*
>>>> + * Bit positions of registers that are defined above, in the sysreg.h style
>>>> + * of _MASK, _SHIFT and BIT().
>>>> + */
>>>
>>> ^^^ not really necessary. Instead the format requirement for below mentioned
>>> CORESIGHT_REG_VAL() macro might be relevant and should be mentioned.
>>>
>>>> +#define TRCIDR0_INSTP0_SHIFT                        1
>>>> +#define TRCIDR0_INSTP0_MASK                 GENMASK(1, 0)
>>>> +#define TRCIDR0_TRCBB                               BIT(5)
>>>> +#define TRCIDR0_TRCCOND                             BIT(6)
>>>> +#define TRCIDR0_TRCCCI                              BIT(7)
>>>> +#define TRCIDR0_RETSTACK                    BIT(9)
>>>> +#define TRCIDR0_NUMEVENT_SHIFT                      10
>>>> +#define TRCIDR0_NUMEVENT_MASK                       GENMASK(1, 0)
>>>> +#define TRCIDR0_QSUPP_SHIFT                 15
>>>> +#define TRCIDR0_QSUPP_MASK                  GENMASK(1, 0)
>>>> +#define TRCIDR0_TSSIZE_SHIFT                        24
>>>> +#define TRCIDR0_TSSIZE_MASK                 GENMASK(4, 0)
>>>> +
>>>>   /*
>>>>    * System instructions to access ETM registers.
>>>>    * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
>>>> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
>>>> index ff1dd2092ac5..1452c6038421 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-priv.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-priv.h
>>>> @@ -36,6 +36,11 @@
>>>>
>>>>   #define TIMEOUT_US         100
>>>>   #define BMVAL(val, lsb, msb)       ((val & GENMASK(msb, lsb)) >> lsb)
>>>> +/*
>>>> + * Extract a field from a register where field is #defined in the form
>>>> + * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
>>>> + */
>>>
>>> Looking at the usage, <register_name> is already embedded in <filed_name>. So
>>> it requires <field_name>_SHIFT and <field_name>_MASK instead. Unless register
>>> name should be passed as separate argument (which actually might be better).
>>>
>>> REG_VAL(etmidr0, TRCIDR0_TSSIZE) ----> REG_VAL(etmidr0, TRCIDR0, TSSIZE)
>>
>> I don't see much difference here. So I am fine either way.
>>
>>>
>>> with some restructuring in the comment ..
>>>
>>> /*
>>>   * Extract a field from a coresight register
>>>   *
>>>   * Required fields are defined as macros like the following
>>>   *
>>>   * <register_name>_<field_name>_MASK and <register_name>_<field_name>_SHIFT
>>>   */
>>>
>>>> +#define REG_VAL(val, field) ((val & (field##_MASK << field##_SHIFT)) >> field##_SHIFT)
>>>
>>> This is too generic to be in a coresight header or it should just be
>>> named CORESIGHT_REG_VAL() instead, making it more specific for here.
>>>
>>> The build should fail in case any required macro definition is absent.
>>> I guess no more fortification is required in case macros are missing.
>>>
>>> However CORESIGHT_REG_VAL() is better placed in <coresight-etm4x.h>
>>> just before all the dependent SHIFT/MASK register field definition
>>> starts.
>>
>> Not necessarily. CORESIGHT_REG_VAL() is a generic function and doesn't
>> have anything specific to do with etm4x. We could reuse that for
>> cleaning up other drivers in CoreSight.
>>
>> Cheers
>> Suzuki
> 
> 
>