perf/arm-cmn: Fixes and updates

It transpires that despite the explicit example to the contrary in the CMN-700 TRM, the "extra device ports" config is in fact a per-XP thing rather than a global one. To that end, rework node IDs yet again to carry around the additional data necessary to decode them properly. At this point the notion of fully decomposing an ID becomes more impractical than it's worth, so unabstracting the XY mesh coordinates (where 2/3 users were just debug anyway) ends up leaving things a bit simpler overall.

Fixes 60d1504070c2 ("perf/arm-cmn: Support new IP features")

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm-cmn.c | 94 ++++++++++++++++++------------------------
 1 file changed, 40 insertions(+), 54 deletions(-)

drivers/perf/arm-cmn.c +40 -54

index c932d9d355cf..fd2122a37f22 100644
@@ -24,14 +24,6 @@
 #define CMN_NI_NODE_ID			GENMASK_ULL(31, 16)
 #define CMN_NI_LOGICAL_ID		GENMASK_ULL(47, 32)
 
-#define CMN_NODEID_DEVID(reg)		((reg) & 3)
-#define CMN_NODEID_EXT_DEVID(reg)	((reg) & 1)
-#define CMN_NODEID_PID(reg)		(((reg) >> 2) & 1)
-#define CMN_NODEID_EXT_PID(reg)		(((reg) >> 1) & 3)
-#define CMN_NODEID_1x1_PID(reg)		(((reg) >> 2) & 7)
-#define CMN_NODEID_X(reg, bits)		((reg) >> (3 + (bits)))
-#define CMN_NODEID_Y(reg, bits)		(((reg) >> 3) & ((1U << (bits)) - 1))
-
 #define CMN_CHILD_INFO			0x0080
 #define CMN_CI_CHILD_COUNT		GENMASK_ULL(15, 0)
 #define CMN_CI_CHILD_PTR_OFFSET		GENMASK_ULL(31, 16)

@@ -280,8 +272,11 @@ struct arm_cmn_node {
 	u16 id, logid;
 	enum cmn_node_type type;
 
+	/* XP properties really, but replicated to children for convenience */
 	u8 dtm;
 	s8 dtc;
+	u8 portid_bits:4;
+	u8 deviceid_bits:4;
 	/* DN/HN-F/CXHA */
 	struct {
 		u8 val : 4;

@@ -357,49 +352,33 @@ struct arm_cmn {
 static int arm_cmn_hp_state;
 
 struct arm_cmn_nodeid {
-	u8 x;
-	u8 y;
 	u8 port;
 	u8 dev;
 };
 
 static int arm_cmn_xyidbits(const struct arm_cmn *cmn)
 {
-	return fls((cmn->mesh_x - 1) | (cmn->mesh_y - 1) | 2);
+	return fls((cmn->mesh_x - 1) | (cmn->mesh_y - 1));
 }
 
-static struct arm_cmn_nodeid arm_cmn_nid(const struct arm_cmn *cmn, u16 id)
+static struct arm_cmn_nodeid arm_cmn_nid(const struct arm_cmn_node *dn)
 {
 	struct arm_cmn_nodeid nid;
 
-	if (cmn->num_xps == 1) {
-		nid.x = 0;
-		nid.y = 0;
-		nid.port = CMN_NODEID_1x1_PID(id);
-		nid.dev = CMN_NODEID_DEVID(id);
-	} else {
-		int bits = arm_cmn_xyidbits(cmn);
-
-		nid.x = CMN_NODEID_X(id, bits);
-		nid.y = CMN_NODEID_Y(id, bits);
-		if (cmn->ports_used & 0xc) {
-			nid.port = CMN_NODEID_EXT_PID(id);
-			nid.dev = CMN_NODEID_EXT_DEVID(id);
-		} else {
-			nid.port = CMN_NODEID_PID(id);
-			nid.dev = CMN_NODEID_DEVID(id);
-		}
-	}
+	nid.dev = dn->id & ((1U << dn->deviceid_bits) - 1);
+	nid.port = (dn->id >> dn->deviceid_bits) & ((1U << dn->portid_bits) - 1);
 	return nid;
 }
 
 static struct arm_cmn_node *arm_cmn_node_to_xp(const struct arm_cmn *cmn,
 					       const struct arm_cmn_node *dn)
 {
-	struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
-	int xp_idx = cmn->mesh_x * nid.y + nid.x;
+	int id = dn->id >> (dn->portid_bits + dn->deviceid_bits);
+	int bits = arm_cmn_xyidbits(cmn);
+	int x = id > bits;
+	int y = id & ((1U << bits) - 1);
 
-	return cmn->xps + xp_idx;
+	return cmn->xps + cmn->mesh_x * y + x;
 }
 static struct arm_cmn_node *arm_cmn_node(const struct arm_cmn *cmn,
 					 enum cmn_node_type type)

@@ -485,13 +464,13 @@ static const char *arm_cmn_device_type(u8 type)
 	}
 }
 
-static void arm_cmn_show_logid(struct seq_file *s, int x, int y, int p, int d)
+static void arm_cmn_show_logid(struct seq_file *s, const struct arm_cmn_node *xp, int p, int d)
 {
 	struct arm_cmn *cmn = s->private;
 	struct arm_cmn_node *dn;
+	u16 id = xp->id | d | (p << xp->deviceid_bits);
 
 	for (dn = cmn->dns; dn->type; dn++) {
-		struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
 		int pad = dn->logid < 10;
 
 		if (dn->type == CMN_TYPE_XP)

@@ -500,7 +479,7 @@ static void arm_cmn_show_logid(struct seq_file *s, int x, int y, int p, int d)
 		if (dn->type < CMN_TYPE_HNI)
 			continue;
 
-		if (nid.x != x || nid.y != y || nid.port != p || nid.dev != d)
+		if (dn->id != id)
 			continue;
 
 		seq_printf(s, " %*c#%-*d  |", pad + 1, ' ', 3 - pad, dn->logid);

@@ -521,6 +500,7 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
 	y = cmn->mesh_y;
 	while (y--) {
 		int xp_base = cmn->mesh_x * y;
+		struct arm_cmn_node *xp = cmn->xps + xp_base;
 		u8 port[CMN_MAX_PORTS][CMN_MAX_DIMENSION];
 
 		for (x = 0; x < cmn->mesh_x; x++)

@@ -528,16 +508,14 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
 
 		seq_printf(s, "\n%-2d   |", y);
 		for (x = 0; x < cmn->mesh_x; x++) {
-			struct arm_cmn_node *xp = cmn->xps + xp_base + x;
-
 			for (p = 0; p < CMN_MAX_PORTS; p++)
-				port[p][x] = arm_cmn_device_connect_info(cmn, xp, p);
+				port[p][x] = arm_cmn_device_connect_info(cmn, xp + x, p);
 			seq_printf(s, " XP #%-3d|", xp_base + x);
 		}
 
 		seq_puts(s, "\n     |");
 		for (x = 0; x < cmn->mesh_x; x++) {
-			s8 dtc = cmn->xps[xp_base + x].dtc;
+			s8 dtc = xp[x].dtc;
 
 			if (dtc < 0)
 				seq_puts(s, " DTC ?? |");

@@ -554,10 +532,10 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
 				seq_puts(s, arm_cmn_device_type(port[p][x]));
 			seq_puts(s, "\n    0|");
 			for (x = 0; x < cmn->mesh_x; x++)
-				arm_cmn_show_logid(s, x, y, p, 0);
+				arm_cmn_show_logid(s, xp + x, p, 0);
 			seq_puts(s, "\n    1|");
 			for (x = 0; x < cmn->mesh_x; x++)
-				arm_cmn_show_logid(s, x, y, p, 1);
+				arm_cmn_show_logid(s, xp + x, p, 1);
 		}
 		seq_puts(s, "\n-----+");
 	}

@@ -1815,10 +1793,7 @@ static int arm_cmn_event_init(struct perf_event *event)
 	}
 
 	if (!hw->num_dns) {
-		struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, nodeid);
-
-		dev_dbg(cmn->dev, "invalid node 0x%x (%d,%d,%d,%d) type 0x%x\n",
-			nodeid, nid.x, nid.y, nid.port, nid.dev, type);
+		dev_dbg(cmn->dev, "invalid node 0x%x type 0x%x\n", nodeid, type);
 		return -EINVAL;
 	}
 

@@ -1921,7 +1896,7 @@ static int arm_cmn_event_add(struct perf_event *event, int flags)
 			arm_cmn_claim_wp_idx(dtm, event, d, wp_idx, i);
 			writel_relaxed(cfg, dtm->base + CMN_DTM_WPn_CONFIG(wp_idx));
 		} else {
-			struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
+			struct arm_cmn_nodeid nid = arm_cmn_nid(dn);
 
 			if (cmn->multi_dtm)
 				nid.port %= 2;

@@ -2168,10 +2143,12 @@ static int arm_cmn_init_dtcs(struct arm_cmn *cmn)
 			continue;
 
 		xp = arm_cmn_node_to_xp(cmn, dn);
+		dn->portid_bits = xp->portid_bits;
+		dn->deviceid_bits = xp->deviceid_bits;
 		dn->dtc = xp->dtc;
 		dn->dtm = xp->dtm;
 		if (cmn->multi_dtm)
-			dn->dtm += arm_cmn_nid(cmn, dn->id).port / 2;
+			dn->dtm += arm_cmn_nid(dn).port / 2;
 
 		if (dn->type == CMN_TYPE_DTC) {
 			int err = arm_cmn_init_dtc(cmn, dn, dtc_idx++);

@@ -2341,18 +2318,27 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 		arm_cmn_init_dtm(dtm++, xp, 0);
 		/*
 		 * Keeping track of connected ports will let us filter out
-		 * unnecessary XP events easily. We can also reliably infer the
-		 * "extra device ports" configuration for the node ID format
-		 * from this, since in that case we will see at least one XP
-		 * with port 2 connected, for the HN-D.
+		 * unnecessary XP events easily, and also infer the per-XP
+		 * part of the node ID format.
 		 */
 		for (int p = 0; p < CMN_MAX_PORTS; p++)
 			if (arm_cmn_device_connect_info(cmn, xp, p))
 				xp_ports |= BIT(p);
 
-		if (cmn->multi_dtm && (xp_ports & 0xc))
+		if (cmn->num_xps == 1) {
+			xp->portid_bits = 3;
+			xp->deviceid_bits = 2;
+		} else if (xp_ports > 0x3) {
+			xp->portid_bits = 2;
+			xp->deviceid_bits = 1;
+		} else {
+			xp->portid_bits = 1;
+			xp->deviceid_bits = 2;
+		}
+
+		if (cmn->multi_dtm && (xp_ports > 0x3))
 			arm_cmn_init_dtm(dtm++, xp, 1);
-		if (cmn->multi_dtm && (xp_ports & 0x30))
+		if (cmn->multi_dtm && (xp_ports > 0xf))
 			arm_cmn_init_dtm(dtm++, xp, 2);
 
 		cmn->ports_used |= xp_ports;

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Hi Robin,

Quoted 10 lines · Robin Murphy

It transpires that despite the explicit example to the contrary in the CMN-700 TRM, the "extra device ports" config is in fact a per-XP thing rather than a global one. To that end, rework node IDs yet again to carry around the additional data necessary to decode them properly. At this point the notion of fully decomposing an ID becomes more impractical than it's worth, so unabstracting the XY mesh coordinates (where 2/3 users were just debug anyway) ends up leaving things a bit simpler overall.

Fixes 60d1504070c2 ("perf/arm-cmn: Support new IP features")

Does this fix an observable functional issue? It's difficult to tell what the impact of this is from the commit message.

i.e. what is the impact:

* On the CMN-700 programmers model?

* As observed by a user of the PMU driver?

... and is there anything in the manual that spells out that this is a per-XP property? I'm struggling to find that in the CMN-700 TRM, as it seems to talk about "mesh configuration(s) with extra device ports".

Mark.

Quoted 232 lines

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-cmn.c | 94 ++++++++++++++++++------------------------
  1 file changed, 40 insertions(+), 54 deletions(-)

drivers/perf/arm-cmn.c +40 -54

index c932d9d355cf..fd2122a37f22 100644
@@ -24,14 +24,6 @@
  #define CMN_NI_NODE_ID			GENMASK_ULL(31, 16)
  #define CMN_NI_LOGICAL_ID		GENMASK_ULL(47, 32)
 
-#define CMN_NODEID_DEVID(reg)		((reg) & 3)
-#define CMN_NODEID_EXT_DEVID(reg)	((reg) & 1)
-#define CMN_NODEID_PID(reg)		(((reg) >> 2) & 1)
-#define CMN_NODEID_EXT_PID(reg)		(((reg) >> 1) & 3)
-#define CMN_NODEID_1x1_PID(reg)		(((reg) >> 2) & 7)
-#define CMN_NODEID_X(reg, bits)		((reg) >> (3 + (bits)))
-#define CMN_NODEID_Y(reg, bits)		(((reg) >> 3) & ((1U << (bits)) - 1))
-
  #define CMN_CHILD_INFO			0x0080
  #define CMN_CI_CHILD_COUNT		GENMASK_ULL(15, 0)
  #define CMN_CI_CHILD_PTR_OFFSET		GENMASK_ULL(31, 16)

@@ -280,8 +272,11 @@ struct arm_cmn_node {
  	u16 id, logid;
  	enum cmn_node_type type;
 
+	/* XP properties really, but replicated to children for convenience */
  	u8 dtm;
  	s8 dtc;
+	u8 portid_bits:4;
+	u8 deviceid_bits:4;
  	/* DN/HN-F/CXHA */
  	struct {
  		u8 val : 4;

@@ -357,49 +352,33 @@ struct arm_cmn {
  static int arm_cmn_hp_state;
 
  struct arm_cmn_nodeid {
-	u8 x;
-	u8 y;
  	u8 port;
  	u8 dev;
  };
 
  static int arm_cmn_xyidbits(const struct arm_cmn *cmn)
  {
-	return fls((cmn->mesh_x - 1) | (cmn->mesh_y - 1) | 2);
+	return fls((cmn->mesh_x - 1) | (cmn->mesh_y - 1));
  }
 
-static struct arm_cmn_nodeid arm_cmn_nid(const struct arm_cmn *cmn, u16 id)
+static struct arm_cmn_nodeid arm_cmn_nid(const struct arm_cmn_node *dn)
  {
  	struct arm_cmn_nodeid nid;
 
-	if (cmn->num_xps == 1) {
-		nid.x = 0;
-		nid.y = 0;
-		nid.port = CMN_NODEID_1x1_PID(id);
-		nid.dev = CMN_NODEID_DEVID(id);
-	} else {
-		int bits = arm_cmn_xyidbits(cmn);
-
-		nid.x = CMN_NODEID_X(id, bits);
-		nid.y = CMN_NODEID_Y(id, bits);
-		if (cmn->ports_used & 0xc) {
-			nid.port = CMN_NODEID_EXT_PID(id);
-			nid.dev = CMN_NODEID_EXT_DEVID(id);
-		} else {
-			nid.port = CMN_NODEID_PID(id);
-			nid.dev = CMN_NODEID_DEVID(id);
-		}
-	}
+	nid.dev = dn->id & ((1U << dn->deviceid_bits) - 1);
+	nid.port = (dn->id >> dn->deviceid_bits) & ((1U << dn->portid_bits) - 1);
  	return nid;
  }
 
  static struct arm_cmn_node *arm_cmn_node_to_xp(const struct arm_cmn *cmn,
  					       const struct arm_cmn_node *dn)
  {
-	struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
-	int xp_idx = cmn->mesh_x * nid.y + nid.x;
+	int id = dn->id >> (dn->portid_bits + dn->deviceid_bits);
+	int bits = arm_cmn_xyidbits(cmn);
+	int x = id > bits;
+	int y = id & ((1U << bits) - 1);
 
-	return cmn->xps + xp_idx;
+	return cmn->xps + cmn->mesh_x * y + x;
  }
  static struct arm_cmn_node *arm_cmn_node(const struct arm_cmn *cmn,
  					 enum cmn_node_type type)

@@ -485,13 +464,13 @@ static const char *arm_cmn_device_type(u8 type)
  	}
  }
 
-static void arm_cmn_show_logid(struct seq_file *s, int x, int y, int p, int d)
+static void arm_cmn_show_logid(struct seq_file *s, const struct arm_cmn_node *xp, int p, int d)
  {
  	struct arm_cmn *cmn = s->private;
  	struct arm_cmn_node *dn;
+	u16 id = xp->id | d | (p << xp->deviceid_bits);
 
  	for (dn = cmn->dns; dn->type; dn++) {
-		struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
  		int pad = dn->logid < 10;
 
  		if (dn->type == CMN_TYPE_XP)

@@ -500,7 +479,7 @@ static void arm_cmn_show_logid(struct seq_file *s, int x, int y, int p, int d)
  		if (dn->type < CMN_TYPE_HNI)
  			continue;
 
-		if (nid.x != x || nid.y != y || nid.port != p || nid.dev != d)
+		if (dn->id != id)
  			continue;
 
  		seq_printf(s, " %*c#%-*d  |", pad + 1, ' ', 3 - pad, dn->logid);

@@ -521,6 +500,7 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
  	y = cmn->mesh_y;
  	while (y--) {
  		int xp_base = cmn->mesh_x * y;
+		struct arm_cmn_node *xp = cmn->xps + xp_base;
  		u8 port[CMN_MAX_PORTS][CMN_MAX_DIMENSION];
 
  		for (x = 0; x < cmn->mesh_x; x++)

@@ -528,16 +508,14 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
 
  		seq_printf(s, "\n%-2d   |", y);
  		for (x = 0; x < cmn->mesh_x; x++) {
-			struct arm_cmn_node *xp = cmn->xps + xp_base + x;
-
  			for (p = 0; p < CMN_MAX_PORTS; p++)
-				port[p][x] = arm_cmn_device_connect_info(cmn, xp, p);
+				port[p][x] = arm_cmn_device_connect_info(cmn, xp + x, p);
  			seq_printf(s, " XP #%-3d|", xp_base + x);
  		}
 
  		seq_puts(s, "\n     |");
  		for (x = 0; x < cmn->mesh_x; x++) {
-			s8 dtc = cmn->xps[xp_base + x].dtc;
+			s8 dtc = xp[x].dtc;
 
  			if (dtc < 0)
  				seq_puts(s, " DTC ?? |");

@@ -554,10 +532,10 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
  				seq_puts(s, arm_cmn_device_type(port[p][x]));
  			seq_puts(s, "\n    0|");
  			for (x = 0; x < cmn->mesh_x; x++)
-				arm_cmn_show_logid(s, x, y, p, 0);
+				arm_cmn_show_logid(s, xp + x, p, 0);
  			seq_puts(s, "\n    1|");
  			for (x = 0; x < cmn->mesh_x; x++)
-				arm_cmn_show_logid(s, x, y, p, 1);
+				arm_cmn_show_logid(s, xp + x, p, 1);
  		}
  		seq_puts(s, "\n-----+");
  	}

@@ -1815,10 +1793,7 @@ static int arm_cmn_event_init(struct perf_event *event)
  	}
 
  	if (!hw->num_dns) {
-		struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, nodeid);
-
-		dev_dbg(cmn->dev, "invalid node 0x%x (%d,%d,%d,%d) type 0x%x\n",
-			nodeid, nid.x, nid.y, nid.port, nid.dev, type);
+		dev_dbg(cmn->dev, "invalid node 0x%x type 0x%x\n", nodeid, type);
  		return -EINVAL;
  	}
 

@@ -1921,7 +1896,7 @@ static int arm_cmn_event_add(struct perf_event *event, int flags)
  			arm_cmn_claim_wp_idx(dtm, event, d, wp_idx, i);
  			writel_relaxed(cfg, dtm->base + CMN_DTM_WPn_CONFIG(wp_idx));
  		} else {
-			struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
+			struct arm_cmn_nodeid nid = arm_cmn_nid(dn);
 
  			if (cmn->multi_dtm)
  				nid.port %= 2;

@@ -2168,10 +2143,12 @@ static int arm_cmn_init_dtcs(struct arm_cmn *cmn)
  			continue;
 
  		xp = arm_cmn_node_to_xp(cmn, dn);
+		dn->portid_bits = xp->portid_bits;
+		dn->deviceid_bits = xp->deviceid_bits;
  		dn->dtc = xp->dtc;
  		dn->dtm = xp->dtm;
  		if (cmn->multi_dtm)
-			dn->dtm += arm_cmn_nid(cmn, dn->id).port / 2;
+			dn->dtm += arm_cmn_nid(dn).port / 2;
 
  		if (dn->type == CMN_TYPE_DTC) {
  			int err = arm_cmn_init_dtc(cmn, dn, dtc_idx++);

@@ -2341,18 +2318,27 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
  		arm_cmn_init_dtm(dtm++, xp, 0);
  		/*
  		 * Keeping track of connected ports will let us filter out
-		 * unnecessary XP events easily. We can also reliably infer the
-		 * "extra device ports" configuration for the node ID format
-		 * from this, since in that case we will see at least one XP
-		 * with port 2 connected, for the HN-D.
+		 * unnecessary XP events easily, and also infer the per-XP
+		 * part of the node ID format.
  		 */
  		for (int p = 0; p < CMN_MAX_PORTS; p++)
  			if (arm_cmn_device_connect_info(cmn, xp, p))
  				xp_ports |= BIT(p);
 
-		if (cmn->multi_dtm && (xp_ports & 0xc))
+		if (cmn->num_xps == 1) {
+			xp->portid_bits = 3;
+			xp->deviceid_bits = 2;
+		} else if (xp_ports > 0x3) {
+			xp->portid_bits = 2;
+			xp->deviceid_bits = 1;
+		} else {
+			xp->portid_bits = 1;
+			xp->deviceid_bits = 2;
+		}
+
+		if (cmn->multi_dtm && (xp_ports > 0x3))
  			arm_cmn_init_dtm(dtm++, xp, 1);
-		if (cmn->multi_dtm && (xp_ports & 0x30))
+		if (cmn->multi_dtm && (xp_ports > 0xf))
  			arm_cmn_init_dtm(dtm++, xp, 2);
 
  		cmn->ports_used |= xp_ports;

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Quoted 19 lines · Mark Rutland

Hi Robin,

Quoted 10 lines · Robin Murphy

It transpires that despite the explicit example to the contrary in the CMN-700 TRM, the "extra device ports" config is in fact a per-XP thing rather than a global one. To that end, rework node IDs yet again to carry around the additional data necessary to decode them properly. At this point the notion of fully decomposing an ID becomes more impractical than it's worth, so unabstracting the XY mesh coordinates (where 2/3 users were just debug anyway) ends up leaving things a bit simpler overall.

Fixes 60d1504070c2 ("perf/arm-cmn: Support new IP features")

Does this fix an observable functional issue? It's difficult to tell what the impact of this is from the commit message.

i.e. what is the impact:

Quoted 3 lines

* On the CMN-700 programmers model?

* As observed by a user of the PMU driver?

Yes, there are two areas where we functionally depend on decoding the bottom 3 bits of a node ID to the individual port/device numbers. One is just for the debugfs nicety, but the other is in arm_cmn_event_add() for setting input_sel to the appropriate event source.

This was revealed once some hardware turned up with a mix of 3-port and 2-port XPs, and events from a node at port 1 device 0 on a 2-porter were not counting. It took a while to get to the "hang on, why does that ID end in 0x4 not 0x2?" moment...

Quoted 3 lines

... and is there anything in the manual that spells out that this is a per-XP property? I'm struggling to find that in the CMN-700 TRM, as it seems to talk about "mesh configuration(s) with extra device ports".

That's the thing, the only suggestion is where example 3-4 strongly implies it's global by going out of its way to demonstrate the 3-port format on a 2-port XP, despite that being the opposite of reality. (And yes, this has been raised with the documentation folks as well.)

Thansk,
Robin.

Quoted 211 lines

Mark.

Quoted 208 lines

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-cmn.c | 94 ++++++++++++++++++------------------------
  1 file changed, 40 insertions(+), 54 deletions(-)

drivers/perf/arm-cmn.c +40 -54

index c932d9d355cf..fd2122a37f22 100644
@@ -24,14 +24,6 @@
  #define CMN_NI_NODE_ID			GENMASK_ULL(31, 16)
  #define CMN_NI_LOGICAL_ID		GENMASK_ULL(47, 32)
-#define CMN_NODEID_DEVID(reg)		((reg) & 3)
-#define CMN_NODEID_EXT_DEVID(reg)	((reg) & 1)
-#define CMN_NODEID_PID(reg)		(((reg) >> 2) & 1)
-#define CMN_NODEID_EXT_PID(reg)		(((reg) >> 1) & 3)
-#define CMN_NODEID_1x1_PID(reg)		(((reg) >> 2) & 7)
-#define CMN_NODEID_X(reg, bits)		((reg) >> (3 + (bits)))
-#define CMN_NODEID_Y(reg, bits)		(((reg) >> 3) & ((1U << (bits)) - 1))
-
  #define CMN_CHILD_INFO			0x0080
  #define CMN_CI_CHILD_COUNT		GENMASK_ULL(15, 0)
  #define CMN_CI_CHILD_PTR_OFFSET		GENMASK_ULL(31, 16)

@@ -280,8 +272,11 @@ struct arm_cmn_node {
  	u16 id, logid;
  	enum cmn_node_type type;
+	/* XP properties really, but replicated to children for convenience */
  	u8 dtm;
  	s8 dtc;
+	u8 portid_bits:4;
+	u8 deviceid_bits:4;
  	/* DN/HN-F/CXHA */
  	struct {
  		u8 val : 4;

@@ -357,49 +352,33 @@ struct arm_cmn {
  static int arm_cmn_hp_state;
  struct arm_cmn_nodeid {
-	u8 x;
-	u8 y;
  	u8 port;
  	u8 dev;
  };
  static int arm_cmn_xyidbits(const struct arm_cmn *cmn)
  {
-	return fls((cmn->mesh_x - 1) | (cmn->mesh_y - 1) | 2);
+	return fls((cmn->mesh_x - 1) | (cmn->mesh_y - 1));
  }
-static struct arm_cmn_nodeid arm_cmn_nid(const struct arm_cmn *cmn, u16 id)
+static struct arm_cmn_nodeid arm_cmn_nid(const struct arm_cmn_node *dn)
  {
  	struct arm_cmn_nodeid nid;
-	if (cmn->num_xps == 1) {
-		nid.x = 0;
-		nid.y = 0;
-		nid.port = CMN_NODEID_1x1_PID(id);
-		nid.dev = CMN_NODEID_DEVID(id);
-	} else {
-		int bits = arm_cmn_xyidbits(cmn);
-
-		nid.x = CMN_NODEID_X(id, bits);
-		nid.y = CMN_NODEID_Y(id, bits);
-		if (cmn->ports_used & 0xc) {
-			nid.port = CMN_NODEID_EXT_PID(id);
-			nid.dev = CMN_NODEID_EXT_DEVID(id);
-		} else {
-			nid.port = CMN_NODEID_PID(id);
-			nid.dev = CMN_NODEID_DEVID(id);
-		}
-	}
+	nid.dev = dn->id & ((1U << dn->deviceid_bits) - 1);
+	nid.port = (dn->id >> dn->deviceid_bits) & ((1U << dn->portid_bits) - 1);
  	return nid;
  }
  static struct arm_cmn_node *arm_cmn_node_to_xp(const struct arm_cmn *cmn,
  					       const struct arm_cmn_node *dn)
  {
-	struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
-	int xp_idx = cmn->mesh_x * nid.y + nid.x;
+	int id = dn->id >> (dn->portid_bits + dn->deviceid_bits);
+	int bits = arm_cmn_xyidbits(cmn);
+	int x = id > bits;
+	int y = id & ((1U << bits) - 1);
-	return cmn->xps + xp_idx;
+	return cmn->xps + cmn->mesh_x * y + x;
  }
  static struct arm_cmn_node *arm_cmn_node(const struct arm_cmn *cmn,
  					 enum cmn_node_type type)

@@ -485,13 +464,13 @@ static const char *arm_cmn_device_type(u8 type)
  	}
  }
-static void arm_cmn_show_logid(struct seq_file *s, int x, int y, int p, int d)
+static void arm_cmn_show_logid(struct seq_file *s, const struct arm_cmn_node *xp, int p, int d)
  {
  	struct arm_cmn *cmn = s->private;
  	struct arm_cmn_node *dn;
+	u16 id = xp->id | d | (p << xp->deviceid_bits);
  	for (dn = cmn->dns; dn->type; dn++) {
-		struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
  		int pad = dn->logid < 10;
  		if (dn->type == CMN_TYPE_XP)

@@ -500,7 +479,7 @@ static void arm_cmn_show_logid(struct seq_file *s, int x, int y, int p, int d)
  		if (dn->type < CMN_TYPE_HNI)
  			continue;
-		if (nid.x != x || nid.y != y || nid.port != p || nid.dev != d)
+		if (dn->id != id)
  			continue;
  		seq_printf(s, " %*c#%-*d  |", pad + 1, ' ', 3 - pad, dn->logid);

@@ -521,6 +500,7 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
  	y = cmn->mesh_y;
  	while (y--) {
  		int xp_base = cmn->mesh_x * y;
+		struct arm_cmn_node *xp = cmn->xps + xp_base;
  		u8 port[CMN_MAX_PORTS][CMN_MAX_DIMENSION];
  		for (x = 0; x < cmn->mesh_x; x++)

@@ -528,16 +508,14 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
  		seq_printf(s, "\n%-2d   |", y);
  		for (x = 0; x < cmn->mesh_x; x++) {
-			struct arm_cmn_node *xp = cmn->xps + xp_base + x;
-
  			for (p = 0; p < CMN_MAX_PORTS; p++)
-				port[p][x] = arm_cmn_device_connect_info(cmn, xp, p);
+				port[p][x] = arm_cmn_device_connect_info(cmn, xp + x, p);
  			seq_printf(s, " XP #%-3d|", xp_base + x);
  		}
  		seq_puts(s, "\n     |");
  		for (x = 0; x < cmn->mesh_x; x++) {
-			s8 dtc = cmn->xps[xp_base + x].dtc;
+			s8 dtc = xp[x].dtc;
  			if (dtc < 0)
  				seq_puts(s, " DTC ?? |");

@@ -554,10 +532,10 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
  				seq_puts(s, arm_cmn_device_type(port[p][x]));
  			seq_puts(s, "\n    0|");
  			for (x = 0; x < cmn->mesh_x; x++)
-				arm_cmn_show_logid(s, x, y, p, 0);
+				arm_cmn_show_logid(s, xp + x, p, 0);
  			seq_puts(s, "\n    1|");
  			for (x = 0; x < cmn->mesh_x; x++)
-				arm_cmn_show_logid(s, x, y, p, 1);
+				arm_cmn_show_logid(s, xp + x, p, 1);
  		}
  		seq_puts(s, "\n-----+");
  	}

@@ -1815,10 +1793,7 @@ static int arm_cmn_event_init(struct perf_event *event)
  	}
  	if (!hw->num_dns) {
-		struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, nodeid);
-
-		dev_dbg(cmn->dev, "invalid node 0x%x (%d,%d,%d,%d) type 0x%x\n",
-			nodeid, nid.x, nid.y, nid.port, nid.dev, type);
+		dev_dbg(cmn->dev, "invalid node 0x%x type 0x%x\n", nodeid, type);
  		return -EINVAL;
  	}

@@ -1921,7 +1896,7 @@ static int arm_cmn_event_add(struct perf_event *event, int flags)
  			arm_cmn_claim_wp_idx(dtm, event, d, wp_idx, i);
  			writel_relaxed(cfg, dtm->base + CMN_DTM_WPn_CONFIG(wp_idx));
  		} else {
-			struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
+			struct arm_cmn_nodeid nid = arm_cmn_nid(dn);
  			if (cmn->multi_dtm)
  				nid.port %= 2;

@@ -2168,10 +2143,12 @@ static int arm_cmn_init_dtcs(struct arm_cmn *cmn)
  			continue;
  		xp = arm_cmn_node_to_xp(cmn, dn);
+		dn->portid_bits = xp->portid_bits;
+		dn->deviceid_bits = xp->deviceid_bits;
  		dn->dtc = xp->dtc;
  		dn->dtm = xp->dtm;
  		if (cmn->multi_dtm)
-			dn->dtm += arm_cmn_nid(cmn, dn->id).port / 2;
+			dn->dtm += arm_cmn_nid(dn).port / 2;
  		if (dn->type == CMN_TYPE_DTC) {
  			int err = arm_cmn_init_dtc(cmn, dn, dtc_idx++);

@@ -2341,18 +2318,27 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
  		arm_cmn_init_dtm(dtm++, xp, 0);
  		/*
  		 * Keeping track of connected ports will let us filter out
-		 * unnecessary XP events easily. We can also reliably infer the
-		 * "extra device ports" configuration for the node ID format
-		 * from this, since in that case we will see at least one XP
-		 * with port 2 connected, for the HN-D.
+		 * unnecessary XP events easily, and also infer the per-XP
+		 * part of the node ID format.
  		 */
  		for (int p = 0; p < CMN_MAX_PORTS; p++)
  			if (arm_cmn_device_connect_info(cmn, xp, p))
  				xp_ports |= BIT(p);
-		if (cmn->multi_dtm && (xp_ports & 0xc))
+		if (cmn->num_xps == 1) {
+			xp->portid_bits = 3;
+			xp->deviceid_bits = 2;
+		} else if (xp_ports > 0x3) {
+			xp->portid_bits = 2;
+			xp->deviceid_bits = 1;
+		} else {
+			xp->portid_bits = 1;
+			xp->deviceid_bits = 2;
+		}
+
+		if (cmn->multi_dtm && (xp_ports > 0x3))
  			arm_cmn_init_dtm(dtm++, xp, 1);
-		if (cmn->multi_dtm && (xp_ports & 0x30))
+		if (cmn->multi_dtm && (xp_ports > 0xf))
  			arm_cmn_init_dtm(dtm++, xp, 2);
  		cmn->ports_used |= xp_ports;

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Quoted 24 lines · Robin Murphy

Quoted 22 lines · Mark Rutland

Hi Robin,

Quoted 10 lines · Robin Murphy

It transpires that despite the explicit example to the contrary in the CMN-700 TRM, the "extra device ports" config is in fact a per-XP thing rather than a global one. To that end, rework node IDs yet again to carry around the additional data necessary to decode them properly. At this point the notion of fully decomposing an ID becomes more impractical than it's worth, so unabstracting the XY mesh coordinates (where 2/3 users were just debug anyway) ends up leaving things a bit simpler overall.

Fixes 60d1504070c2 ("perf/arm-cmn: Support new IP features")

Does this fix an observable functional issue? It's difficult to tell what the impact of this is from the commit message.

i.e. what is the impact:

* On the CMN-700 programmers model?

* As observed by a user of the PMU driver?

Quoted 4 lines

Yes, there are two areas where we functionally depend on decoding the bottom 3 bits of a node ID to the individual port/device numbers. One is just for the debugfs nicety, but the other is in arm_cmn_event_add() for setting input_sel to the appropriate event source.

Cool; and does treating this wrong just result in getting bad values out in sysfs/perf, or is it possible that we have something like memory corruption?

Quoted 4 lines

This was revealed once some hardware turned up with a mix of 3-port and 2-port XPs, and events from a node at port 1 device 0 on a 2-porter were not counting. It took a while to get to the "hang on, why does that ID end in 0x4 not 0x2?" moment...

I see; so we were mislead by the documentation, then saw HW which demonstrated this.

It'd be nice to say something like:

The CMN-700 TRM implies that the "extra device ports" configuration is global to a CMN instance and the CMN PMU driver currently assumes this. Unfortunately this is not correct as this configuration is per-XP, and hardware exists where some XPs have extra device ports while others do not.

The presence of extra decice ports affects how the bottom 3 bits of a node ID map to individual port/device numbers, and when the driver misinterprets this, it may expose incorrect information in syfs, and arm_cmn_event_add() may configure input_sel incorrectly, resulting in incorrect performance counter numbers.

Fix this by reworking node IDs to carry the additional data necessary to decode them properly. At this point the notion of fully decomposing an ID becomes more impractical than it's worth, so unabstracting the XY mesh coordinates (where 2/3 users were just debug anyway) ends up leaving things a bit simpler overall.

With that wording (or something very similar), this looks good to me, and with that:

Acked-by Mark Rutland <[email protected]>

Mark.

Quoted 223 lines

Quoted 3 lines

... and is there anything in the manual that spells out that this is a per-XP property? I'm struggling to find that in the CMN-700 TRM, as it seems to talk about "mesh configuration(s) with extra device ports".

That's the thing, the only suggestion is where example 3-4 strongly implies it's global by going out of its way to demonstrate the 3-port format on a 2-port XP, despite that being the opposite of reality. (And yes, this has been raised with the documentation folks as well.)

Thansk,
Robin.

Quoted 211 lines

Mark.

Quoted 208 lines

Signed-off-by Robin Murphy <[email protected]>

---
   drivers/perf/arm-cmn.c | 94 ++++++++++++++++++------------------------
   1 file changed, 40 insertions(+), 54 deletions(-)

drivers/perf/arm-cmn.c +40 -54

index c932d9d355cf..fd2122a37f22 100644
@@ -24,14 +24,6 @@
   #define CMN_NI_NODE_ID			GENMASK_ULL(31, 16)
   #define CMN_NI_LOGICAL_ID		GENMASK_ULL(47, 32)
-#define CMN_NODEID_DEVID(reg)		((reg) & 3)
-#define CMN_NODEID_EXT_DEVID(reg)	((reg) & 1)
-#define CMN_NODEID_PID(reg)		(((reg) >> 2) & 1)
-#define CMN_NODEID_EXT_PID(reg)		(((reg) >> 1) & 3)
-#define CMN_NODEID_1x1_PID(reg)		(((reg) >> 2) & 7)
-#define CMN_NODEID_X(reg, bits)		((reg) >> (3 + (bits)))
-#define CMN_NODEID_Y(reg, bits)		(((reg) >> 3) & ((1U << (bits)) - 1))
-
   #define CMN_CHILD_INFO			0x0080
   #define CMN_CI_CHILD_COUNT		GENMASK_ULL(15, 0)
   #define CMN_CI_CHILD_PTR_OFFSET		GENMASK_ULL(31, 16)

@@ -280,8 +272,11 @@ struct arm_cmn_node {
   	u16 id, logid;
   	enum cmn_node_type type;
+	/* XP properties really, but replicated to children for convenience */
   	u8 dtm;
   	s8 dtc;
+	u8 portid_bits:4;
+	u8 deviceid_bits:4;
   	/* DN/HN-F/CXHA */
   	struct {
   		u8 val : 4;

@@ -357,49 +352,33 @@ struct arm_cmn {
   static int arm_cmn_hp_state;
   struct arm_cmn_nodeid {
-	u8 x;
-	u8 y;
   	u8 port;
   	u8 dev;
   };
   static int arm_cmn_xyidbits(const struct arm_cmn *cmn)
   {
-	return fls((cmn->mesh_x - 1) | (cmn->mesh_y - 1) | 2);
+	return fls((cmn->mesh_x - 1) | (cmn->mesh_y - 1));
   }
-static struct arm_cmn_nodeid arm_cmn_nid(const struct arm_cmn *cmn, u16 id)
+static struct arm_cmn_nodeid arm_cmn_nid(const struct arm_cmn_node *dn)
   {
   	struct arm_cmn_nodeid nid;
-	if (cmn->num_xps == 1) {
-		nid.x = 0;
-		nid.y = 0;
-		nid.port = CMN_NODEID_1x1_PID(id);
-		nid.dev = CMN_NODEID_DEVID(id);
-	} else {
-		int bits = arm_cmn_xyidbits(cmn);
-
-		nid.x = CMN_NODEID_X(id, bits);
-		nid.y = CMN_NODEID_Y(id, bits);
-		if (cmn->ports_used & 0xc) {
-			nid.port = CMN_NODEID_EXT_PID(id);
-			nid.dev = CMN_NODEID_EXT_DEVID(id);
-		} else {
-			nid.port = CMN_NODEID_PID(id);
-			nid.dev = CMN_NODEID_DEVID(id);
-		}
-	}
+	nid.dev = dn->id & ((1U << dn->deviceid_bits) - 1);
+	nid.port = (dn->id >> dn->deviceid_bits) & ((1U << dn->portid_bits) - 1);
   	return nid;
   }
   static struct arm_cmn_node *arm_cmn_node_to_xp(const struct arm_cmn *cmn,
   					       const struct arm_cmn_node *dn)
   {
-	struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
-	int xp_idx = cmn->mesh_x * nid.y + nid.x;
+	int id = dn->id >> (dn->portid_bits + dn->deviceid_bits);
+	int bits = arm_cmn_xyidbits(cmn);
+	int x = id > bits;
+	int y = id & ((1U << bits) - 1);
-	return cmn->xps + xp_idx;
+	return cmn->xps + cmn->mesh_x * y + x;
   }
   static struct arm_cmn_node *arm_cmn_node(const struct arm_cmn *cmn,
   					 enum cmn_node_type type)

@@ -485,13 +464,13 @@ static const char *arm_cmn_device_type(u8 type)
   	}
   }
-static void arm_cmn_show_logid(struct seq_file *s, int x, int y, int p, int d)
+static void arm_cmn_show_logid(struct seq_file *s, const struct arm_cmn_node *xp, int p, int d)
   {
   	struct arm_cmn *cmn = s->private;
   	struct arm_cmn_node *dn;
+	u16 id = xp->id | d | (p << xp->deviceid_bits);
   	for (dn = cmn->dns; dn->type; dn++) {
-		struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
   		int pad = dn->logid < 10;
   		if (dn->type == CMN_TYPE_XP)

@@ -500,7 +479,7 @@ static void arm_cmn_show_logid(struct seq_file *s, int x, int y, int p, int d)
   		if (dn->type < CMN_TYPE_HNI)
   			continue;
-		if (nid.x != x || nid.y != y || nid.port != p || nid.dev != d)
+		if (dn->id != id)
   			continue;
   		seq_printf(s, " %*c#%-*d  |", pad + 1, ' ', 3 - pad, dn->logid);

@@ -521,6 +500,7 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
   	y = cmn->mesh_y;
   	while (y--) {
   		int xp_base = cmn->mesh_x * y;
+		struct arm_cmn_node *xp = cmn->xps + xp_base;
   		u8 port[CMN_MAX_PORTS][CMN_MAX_DIMENSION];
   		for (x = 0; x < cmn->mesh_x; x++)

@@ -528,16 +508,14 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
   		seq_printf(s, "\n%-2d   |", y);
   		for (x = 0; x < cmn->mesh_x; x++) {
-			struct arm_cmn_node *xp = cmn->xps + xp_base + x;
-
   			for (p = 0; p < CMN_MAX_PORTS; p++)
-				port[p][x] = arm_cmn_device_connect_info(cmn, xp, p);
+				port[p][x] = arm_cmn_device_connect_info(cmn, xp + x, p);
   			seq_printf(s, " XP #%-3d|", xp_base + x);
   		}
   		seq_puts(s, "\n     |");
   		for (x = 0; x < cmn->mesh_x; x++) {
-			s8 dtc = cmn->xps[xp_base + x].dtc;
+			s8 dtc = xp[x].dtc;
   			if (dtc < 0)
   				seq_puts(s, " DTC ?? |");

@@ -554,10 +532,10 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
   				seq_puts(s, arm_cmn_device_type(port[p][x]));
   			seq_puts(s, "\n    0|");
   			for (x = 0; x < cmn->mesh_x; x++)
-				arm_cmn_show_logid(s, x, y, p, 0);
+				arm_cmn_show_logid(s, xp + x, p, 0);
   			seq_puts(s, "\n    1|");
   			for (x = 0; x < cmn->mesh_x; x++)
-				arm_cmn_show_logid(s, x, y, p, 1);
+				arm_cmn_show_logid(s, xp + x, p, 1);
   		}
   		seq_puts(s, "\n-----+");
   	}

@@ -1815,10 +1793,7 @@ static int arm_cmn_event_init(struct perf_event *event)
   	}
   	if (!hw->num_dns) {
-		struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, nodeid);
-
-		dev_dbg(cmn->dev, "invalid node 0x%x (%d,%d,%d,%d) type 0x%x\n",
-			nodeid, nid.x, nid.y, nid.port, nid.dev, type);
+		dev_dbg(cmn->dev, "invalid node 0x%x type 0x%x\n", nodeid, type);
   		return -EINVAL;
   	}

@@ -1921,7 +1896,7 @@ static int arm_cmn_event_add(struct perf_event *event, int flags)
   			arm_cmn_claim_wp_idx(dtm, event, d, wp_idx, i);
   			writel_relaxed(cfg, dtm->base + CMN_DTM_WPn_CONFIG(wp_idx));
   		} else {
-			struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
+			struct arm_cmn_nodeid nid = arm_cmn_nid(dn);
   			if (cmn->multi_dtm)
   				nid.port %= 2;

@@ -2168,10 +2143,12 @@ static int arm_cmn_init_dtcs(struct arm_cmn *cmn)
   			continue;
   		xp = arm_cmn_node_to_xp(cmn, dn);
+		dn->portid_bits = xp->portid_bits;
+		dn->deviceid_bits = xp->deviceid_bits;
   		dn->dtc = xp->dtc;
   		dn->dtm = xp->dtm;
   		if (cmn->multi_dtm)
-			dn->dtm += arm_cmn_nid(cmn, dn->id).port / 2;
+			dn->dtm += arm_cmn_nid(dn).port / 2;
   		if (dn->type == CMN_TYPE_DTC) {
   			int err = arm_cmn_init_dtc(cmn, dn, dtc_idx++);

@@ -2341,18 +2318,27 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
   		arm_cmn_init_dtm(dtm++, xp, 0);
   		/*
   		 * Keeping track of connected ports will let us filter out
-		 * unnecessary XP events easily. We can also reliably infer the
-		 * "extra device ports" configuration for the node ID format
-		 * from this, since in that case we will see at least one XP
-		 * with port 2 connected, for the HN-D.
+		 * unnecessary XP events easily, and also infer the per-XP
+		 * part of the node ID format.
   		 */
   		for (int p = 0; p < CMN_MAX_PORTS; p++)
   			if (arm_cmn_device_connect_info(cmn, xp, p))
   				xp_ports |= BIT(p);
-		if (cmn->multi_dtm && (xp_ports & 0xc))
+		if (cmn->num_xps == 1) {
+			xp->portid_bits = 3;
+			xp->deviceid_bits = 2;
+		} else if (xp_ports > 0x3) {
+			xp->portid_bits = 2;
+			xp->deviceid_bits = 1;
+		} else {
+			xp->portid_bits = 1;
+			xp->deviceid_bits = 2;
+		}
+
+		if (cmn->multi_dtm && (xp_ports > 0x3))
   			arm_cmn_init_dtm(dtm++, xp, 1);
-		if (cmn->multi_dtm && (xp_ports & 0x30))
+		if (cmn->multi_dtm && (xp_ports > 0xf))
   			arm_cmn_init_dtm(dtm++, xp, 2);
   		cmn->ports_used |= xp_ports;

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

It transpires that despite the explicit example to the contrary in the CMN-700 TRM, the "extra device ports" config is in fact a per-XP thing rather than a global one. To that end, rework node IDs yet again to carry around the additional data necessary to decode them properly. At this point the notion of fully decomposing an ID becomes more impractical than it's worth, so unabstracting the XY mesh coordinates (where 2/3 users were just debug anyway) ends up leaving things a bit simpler overall.

Fixes 60d1504070c2 ("perf/arm-cmn: Support new IP features")

Signed-off-by Robin Murphy <[email protected]>

---
drivers/perf/arm-cmn.c | 94 ++++++++++++++++++------------------------

1 file changed, 40 insertions(+), 54 deletions(-)

drivers/perf/arm-cmn.c +4 -22

index c932d9d355cf..fd2122a37f22 100644
@@ -357,49 +352,33 @@ struct arm_cmn {
 static int arm_cmn_hp_state;
 
 struct arm_cmn_nodeid {
-	u8 x;
-	u8 y;
 	u8 port;
 	u8 dev;
 };
 
 static int arm_cmn_xyidbits(const struct arm_cmn *cmn)
 {
-	return fls((cmn->mesh_x - 1) | (cmn->mesh_y - 1) | 2);
+	return fls((cmn->mesh_x - 1) | (cmn->mesh_y - 1));
 }
 
-static struct arm_cmn_nodeid arm_cmn_nid(const struct arm_cmn *cmn, u16 id)
+static struct arm_cmn_nodeid arm_cmn_nid(const struct arm_cmn_node *dn)
 {
 	struct arm_cmn_nodeid nid;
 
-	if (cmn->num_xps == 1) {
-		nid.x = 0;
-		nid.y = 0;
-		nid.port = CMN_NODEID_1x1_PID(id);
-		nid.dev = CMN_NODEID_DEVID(id);
-	} else {
-		int bits = arm_cmn_xyidbits(cmn);
-
-		nid.x = CMN_NODEID_X(id, bits);
-		nid.y = CMN_NODEID_Y(id, bits);
-		if (cmn->ports_used & 0xc) {
-			nid.port = CMN_NODEID_EXT_PID(id);
-			nid.dev = CMN_NODEID_EXT_DEVID(id);
-		} else {
-			nid.port = CMN_NODEID_PID(id);
-			nid.dev = CMN_NODEID_DEVID(id);
-		}
-	}
+	nid.dev = dn->id & ((1U << dn->deviceid_bits) - 1);
+	nid.port = (dn->id >> dn->deviceid_bits) & ((1U << dn->portid_bits) - 1);
 	return nid;

drivers/perf/arm-cmn.c +2 -1

+	int y = id & ((1U << bits) - 1);
 
-	return cmn->xps + xp_idx;
+	return cmn->xps + cmn->mesh_x * y + x;
 }
 static struct arm_cmn_node *arm_cmn_node(const struct arm_cmn *cmn,
 					 enum cmn_node_type type)

Quoted 8 lines

static struct arm_cmn_node *arm_cmn_node_to_xp(const struct arm_cmn *cmn, const struct arm_cmn_node *dn)
{
-    struct arm_cmn_nodeid nid = arm_cmn_nid(cmn, dn->id);
-    int xp_idx = cmn->mesh_x * nid.y + nid.x;
+    int id = dn->id >> (dn->portid_bits + dn->deviceid_bits); +    int bits = arm_cmn_xyidbits(cmn);
+    int x = id > bits;

                  ^^^

Instead of comparison, shouldn't that be bit shift?

Apparently pmu_event_sel is offset by 8 for all CCLA nodes, not just the CCLA_RNI combination type.

Fixes 23760a014417 ("perf/arm-cmn: Add CMN-700 support")

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm-cmn.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/perf/arm-cmn.c +0 -0

index fd2122a37f22..0e2e12e2f4fb 100644
@@ -2393,10 +2393,13 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 			case CMN_TYPE_CXHA:

drivers/perf/arm-cmn.c +0 -0

 			/* Nothing to see here */
 			case CMN_TYPE_MPAM_S:
 			case CMN_TYPE_MPAM_NS:

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Quoted 2 lines · Robin Murphy

Apparently pmu_event_sel is offset by 8 for all CCLA nodes, not just the CCLA_RNI combination type.

Was there some reason we used to think that was specific to CCLA_RNI nodes, or was that just an oversight?

Looking at the CMN-700 TRM and scanning for pmu_event_sel, we have:

	16'h2000	por_ccg_ha_pmu_event_sel
	16'h2000	por_ccg_ra_pmu_event_sel
	16'h2008	por_ccla_pmu_event_sel
	16'h2000	por_dn_pmu_event_sel
	16'h2000	cmn_hns_pmu_event_sel
	16'h2000	por_hni_pmu_event_sel
	16'h2008	por_hnp_pmu_event_sel
	16'h2000	por_mxp_pmu_event_sel
	16'h2000	por_rnd_pmu_event_sel
	16'h2000	por_rni_pmu_event_sel
	16'h2000	por_sbsx_pmu_event_sel

Quoted 12 lines

Fixes 23760a014417 ("perf/arm-cmn: Add CMN-700 support")

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-cmn.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

drivers/perf/arm-cmn.c +0 -0

index fd2122a37f22..0e2e12e2f4fb 100644
@@ -2393,10 +2393,13 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
  			case CMN_TYPE_CXHA:

drivers/perf/arm-cmn.c static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset) · line 2402

Quoted 10 lines

drivers/perf/arm-cmn.c +4 -1

  			case CMN_TYPE_CCRA:
  			case CMN_TYPE_CCHA:
-			case CMN_TYPE_CCLA:
  			case CMN_TYPE_HNS:
  				dn++;
  				break;
+			case CMN_TYPE_CCLA:
+				dn->pmu_base += CMN_HNP_PMU_EVENT_SEL;
+				dn++;
+				break;

When reading this for the first time, it looks like a copy-paste error since CMN_HNP_PMU_EVENT_SEL doesn't have any obvious relationship with CCLA nodes.

I reckon it'd be worth adding CMN_CCLA_PMU_EVENT_SEL, and replacing the existing comment above the definition of CMN_HNP_PMU_EVENT_SEL, e.g.

/*
* Some nodes place common registers at different offsets from most
* other nodes.
*/
#define CMN_HNP_PMU_EVENT_SEL 0x008
#define CMN_CCLA_PMU_EVENT_SEL 0x008

That way the switch looks less suspicious, and the comment is a bit more helpful to anyone trying to figure out what's going on here.

With that:

Acked-by Mark Rutland <[email protected]>

Mark.

Quoted 6 lines

drivers/perf/arm-cmn.c +0 -0

  			/* Nothing to see here */
  			case CMN_TYPE_MPAM_S:
  			case CMN_TYPE_MPAM_NS:

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Quoted 6 lines · Mark Rutland

Quoted 2 lines · Robin Murphy

Apparently pmu_event_sel is offset by 8 for all CCLA nodes, not just the CCLA_RNI combination type.

Was there some reason we used to think that was specific to CCLA_RNI nodes, or was that just an oversight?

I imagine it was just oversight/confusion helped by the original r0p0 TRM listing both a por_ccla_pmu_event_sel and a por_ccla_rni_pmu_event_sel as CCLA registers, which I could well believe I misread at a glance while scrolling up and down.

Quoted 52 lines

Looking at the CMN-700 TRM and scanning for pmu_event_sel, we have:

	16'h2000	por_ccg_ha_pmu_event_sel
	16'h2000	por_ccg_ra_pmu_event_sel
	16'h2008	por_ccla_pmu_event_sel
	16'h2000	por_dn_pmu_event_sel
	16'h2000	cmn_hns_pmu_event_sel
	16'h2000	por_hni_pmu_event_sel
	16'h2008	por_hnp_pmu_event_sel
	16'h2000	por_mxp_pmu_event_sel
	16'h2000	por_rnd_pmu_event_sel
	16'h2000	por_rni_pmu_event_sel
	16'h2000	por_sbsx_pmu_event_sel

Quoted 13 lines

Fixes 23760a014417 ("perf/arm-cmn: Add CMN-700 support")

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-cmn.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

drivers/perf/arm-cmn.c +0 -0

index fd2122a37f22..0e2e12e2f4fb 100644
@@ -2393,10 +2393,13 @@ static int arm_cmn_discover(struct arm_cmn *cmn, 
 unsigned int rgn_offset)
  			case CMN_TYPE_CXHA:

drivers/perf/arm-cmn.c static int arm_cmn_discover(struct arm_cmn *cmn, · line 2403

Quoted 10 lines

drivers/perf/arm-cmn.c +4 -1

  			case CMN_TYPE_CCRA:
  			case CMN_TYPE_CCHA:
-			case CMN_TYPE_CCLA:
  			case CMN_TYPE_HNS:
  				dn++;
  				break;
+			case CMN_TYPE_CCLA:
+				dn->pmu_base += CMN_HNP_PMU_EVENT_SEL;
+				dn++;
+				break;

When reading this for the first time, it looks like a copy-paste error since CMN_HNP_PMU_EVENT_SEL doesn't have any obvious relationship with CCLA nodes.

I reckon it'd be worth adding CMN_CCLA_PMU_EVENT_SEL, and replacing the existing comment above the definition of CMN_HNP_PMU_EVENT_SEL, e.g.

/*
* Some nodes place common registers at different offsets from most
* other nodes.
*/
#define CMN_HNP_PMU_EVENT_SEL 0x008
#define CMN_CCLA_PMU_EVENT_SEL 0x008

Quoted 2 lines

That way the switch looks less suspicious, and the comment is a bit more helpful to anyone trying to figure out what's going on here.

Sure, that's a reasonable argument.

Quoted 3 lines

With that:

Acked-by Mark Rutland <[email protected]>

Thanks,
Robin.

Quoted 10 lines

Mark.

Quoted 6 lines

drivers/perf/arm-cmn.c +0 -0

  			/* Nothing to see here */
  			case CMN_TYPE_MPAM_S:

case CMN_TYPE_MPAM_NS:

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

While CMN_MAX_DIMENSION was bumped to 12 for CMN-650, that only supports up to a 10x10 mesh, so bumping dtm_idx to 256 bits at the time worked out OK in practice. However CMN-700 did finally support up to 144 XPs, and thus needs a worst-case 288 bits of dtm_idx for an aggregated XP event on a maxed-out config. Oops.

Fixes 23760a014417 ("perf/arm-cmn: Add CMN-700 support")

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm-cmn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/perf/arm-cmn.c +0 -0

index 0e2e12e2f4fb..c9a2b21a7aec 100644

drivers/perf/arm-cmn.c +0 -0

 	s8 dtc_idx[CMN_MAX_DTCS];
 	u8 num_dns;
 	u8 dtm_offset;

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Quoted 18 lines · Robin Murphy

While CMN_MAX_DIMENSION was bumped to 12 for CMN-650, that only supports up to a 10x10 mesh, so bumping dtm_idx to 256 bits at the time worked out OK in practice. However CMN-700 did finally support up to 144 XPs, and thus needs a worst-case 288 bits of dtm_idx for an aggregated XP event on a maxed-out config. Oops.

Fixes 23760a014417 ("perf/arm-cmn: Add CMN-700 support")

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-cmn.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/perf/arm-cmn.c +0 -0

index 0e2e12e2f4fb..c9a2b21a7aec 100644
@@ -563,7 +563,7 @@ static void arm_cmn_debugfs_init(struct arm_cmn *cmn, int id) {}
 

drivers/perf/arm-cmn.c static void arm_cmn_debugfs_init(struct arm_cmn *cmn, int id) {} · line 566

Quoted 4 lines

drivers/perf/arm-cmn.c +1 -1

  struct arm_cmn_hw_event {
  	struct arm_cmn_node *dn;
-	u64 dtm_idx[4];
+	u64 dtm_idx[5];

Can't we size this based on CMN_MAX_DIMENSION (or CMN_MAX_XPS or CMN_MAX_DTMS), to make that clear?

From the desciription in the commit message it sounds like you need 2 *
CMN_MAX_XPS bits, i.e.

	#define DTM_IDX_BITS	(2 * CMN_MAX_XPS)

	u64 dtm_idx[DIV_ROUND_UP(DTM_IDX_BITS, 64)];

Mark.

Quoted 6 lines

drivers/perf/arm-cmn.c +0 -0

  	s8 dtc_idx[CMN_MAX_DTCS];
  	u8 num_dns;
  	u8 dtm_offset;

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Quoted 13 lines · Mark Rutland

Quoted 12 lines · Robin Murphy

While CMN_MAX_DIMENSION was bumped to 12 for CMN-650, that only supports up to a 10x10 mesh, so bumping dtm_idx to 256 bits at the time worked out OK in practice. However CMN-700 did finally support up to 144 XPs, and thus needs a worst-case 288 bits of dtm_idx for an aggregated XP event on a maxed-out config. Oops.

Fixes 23760a014417 ("perf/arm-cmn: Add CMN-700 support")

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-cmn.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

drivers/perf/arm-cmn.c static void arm_cmn_debugfs_init(struct arm_cmn *cmn, int id) {} · line 565

Quoted 9 lines

drivers/perf/arm-cmn.c +1 -1

index 0e2e12e2f4fb..c9a2b21a7aec 100644
@@ -563,7 +563,7 @@ static void arm_cmn_debugfs_init(struct arm_cmn *cmn, int id) {}
  struct arm_cmn_hw_event {
  	struct arm_cmn_node *dn;
-	u64 dtm_idx[4];
+	u64 dtm_idx[5];

Can't we size this based on CMN_MAX_DIMENSION (or CMN_MAX_XPS or CMN_MAX_DTMS), to make that clear?

Fair enough, I did go back and forth a little on that idea, but reached the opposite conclusion that documenting this with the assert to deliberately make it *not* look straightforward was nicer than wrestling with an accurate name for the logical quantity here, which strictly would be something like CMN_MAX_NODES_PER_TYPE_WE_CARE_ABOUT (there can already be up to 256 RN-Fs, but those aren't visible to the PMU).

I'll have another think on that approach - I do concur that the assert isn't *functionally* any better than automatically sizing the array.

Quoted 13 lines · Mark Rutland

Quoted 12 lines · Robin Murphy

While CMN_MAX_DIMENSION was bumped to 12 for CMN-650, that only supports up to a 10x10 mesh, so bumping dtm_idx to 256 bits at the time worked out OK in practice. However CMN-700 did finally support up to 144 XPs, and thus needs a worst-case 288 bits of dtm_idx for an aggregated XP event on a maxed-out config. Oops.

Fixes 23760a014417 ("perf/arm-cmn: Add CMN-700 support")

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-cmn.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 13 lines

drivers/perf/arm-cmn.c static void arm_cmn_debugfs_init(struct arm_cmn *cmn, · line 566

Quoted 10 lines

drivers/perf/arm-cmn.c +1 -1

index 0e2e12e2f4fb..c9a2b21a7aec 100644
@@ -563,7 +563,7 @@ static void arm_cmn_debugfs_init(struct arm_cmn *cmn, 
 int id) {}
    struct arm_cmn_hw_event {
  	struct arm_cmn_node *dn;
-	u64 dtm_idx[4];
+	u64 dtm_idx[5];

Can't we size this based on CMN_MAX_DIMENSION (or CMN_MAX_XPS or CMN_MAX_DTMS), to make that clear?

Fair enough, I did go back and forth a little on that idea, but reached the opposite conclusion that documenting this with the assert to deliberately make it *not* look straightforward was nicer than wrestling with an accurate name for the logical quantity here, which strictly would be something like CMN_MAX_NODES_PER_TYPE_WE_CARE_ABOUT (there can already be up to 256 RN-Fs, but those aren't visible to the PMU).

I'll have another think on that approach - I do concur that the assert isn't *functionally* any better than automatically sizing the array.

Thanks,
Robin.

Quoted 15 lines

From the desciription in the commit message it sounds like you need 2 *
CMN_MAX_XPS bits, i.e.

	#define DTM_IDX_BITS	(2 * CMN_MAX_XPS)

	u64 dtm_idx[DIV_ROUND_UP(DTM_IDX_BITS, 64)];

Mark.

Quoted 6 lines

drivers/perf/arm-cmn.c +0 -0

  	s8 dtc_idx[CMN_MAX_DTCS];
  	u8 num_dns;
  	u8 dtm_offset;

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

These days we can use static_assert() in the logical place rather than jamming a BUILD_BUG_ON() into the nearest function scope. And since it is now so convenient to do so, let's add an extra one to reinforce the dtm_idx bitmap as well.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm-cmn.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/perf/arm-cmn.c +0 -0

index c9a2b21a7aec..8f7a1a6f8ab7 100644

drivers/perf/arm-cmn.c +0 -1

 #define for_each_hw_dn(hw, dn, i) \
 	for (i = 0, dn = hw->dn; i < hw->num_dns; i++, dn++)

@@ -587,7 +590,6 @@ struct arm_cmn_hw_event {
 
 static struct arm_cmn_hw_event *to_cmn_hw(struct perf_event *event)
 {
-	BUILD_BUG_ON(sizeof(struct arm_cmn_hw_event) > offsetof(struct hw_perf_event, target));
 	return (struct arm_cmn_hw_event *)&event->hw;
 }
 

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

By default, CMN has automatic clock-gating with the implication that a DTC's cycle counter may not increment while the domain is sufficiently idle.

Given that we may have up to 4 DTCs to choose from when scheduling a cycles event, this may potentially lead to surprising results if trying to measure metrics based on activity in a different DTC domain from where cycles end up being counted. Make the reasonable assumption that if the user wants to count cycles, they almost certainly want to count all of the cycles, and disable clock gating while a DTC's cycle counter is in use.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm-cmn.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

drivers/perf/arm-cmn.c +11 -5

index 8f7a1a6f8ab7..4d128db2040c 100644
@@ -115,6 +115,7 @@
 /* The DTC node is where the magic happens */
 #define CMN_DT_DTC_CTL			0x0a00
 #define CMN_DT_DTC_CTL_DT_EN		BIT(0)
+#define CMN_DT_DTC_CTL_CG_DISABLE	BIT(10)
 
 /* DTC counters are paired in 64-bit registers on a 16-byte stride. Yuck */
 #define _CMN_DT_CNT_REG(n)		((((n) / 2) * 4 + (n) % 2) * 4)

@@ -1544,9 +1545,12 @@ static void arm_cmn_event_start(struct perf_event *event, int flags)
 	int i;
 
 	if (type == CMN_TYPE_DTC) {
-		i = hw->dtc_idx[0];
-		writeq_relaxed(CMN_CC_INIT, cmn->dtc[i].base + CMN_DT_PMCCNTR);
-		cmn->dtc[i].cc_active = true;
+		struct arm_cmn_dtc *dtc = cmn->dtc + hw->dtc_idx[0];
+
+		writel_relaxed(CMN_DT_DTC_CTL_DT_EN | CMN_DT_DTC_CTL_CG_DISABLE,
+			       dtc->base + CMN_DT_DTC_CTL);
+		writeq_relaxed(CMN_CC_INIT, dtc->base + CMN_DT_PMCCNTR);
+		dtc->cc_active = true;
 	} else if (type == CMN_TYPE_WP) {
 		u64 val = CMN_EVENT_WP_VAL(event);
 		u64 mask = CMN_EVENT_WP_MASK(event);

@@ -1575,8 +1579,10 @@ static void arm_cmn_event_stop(struct perf_event *event, int flags)
 	int i;
 
 	if (type == CMN_TYPE_DTC) {
-		i = hw->dtc_idx[0];
-		cmn->dtc[i].cc_active = false;
+		struct arm_cmn_dtc *dtc = cmn->dtc + hw->dtc_idx[0];
+
+		dtc->cc_active = false;
+		writel_relaxed(CMN_DT_DTC_CTL_DT_EN, dtc->base + CMN_DT_DTC_CTL);
 	} else if (type == CMN_TYPE_WP) {
 		for_each_hw_dn(hw, dn, i) {
 			void __iomem *base = dn->pmu_base + CMN_DTM_OFFSET(hw->dtm_offset);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Quoted 3 lines · Robin Murphy

By default, CMN has automatic clock-gating with the implication that a DTC's cycle counter may not increment while the domain is sufficiently idle.

Similar is true for the cycles event on the CPU side, so this has some precedent.

Quoted 7 lines

Given that we may have up to 4 DTCs to choose from when scheduling a cycles event, this may potentially lead to surprising results if trying to measure metrics based on activity in a different DTC domain from where cycles end up being counted. Make the reasonable assumption that if the user wants to count cycles, they almost certainly want to count all of the cycles, and disable clock gating while a DTC's cycle counter is in use.

As above, the default does match the CPU side behaviour,

and a user
might be trying to determine how much clock gating occurs over some period, so it's not necessarily right to always disable clock gating. That might need to be an explicit option on the cycles event.

Do we always have the ability to disable clock gating, or can that be locked down by system integration or FW?

Mark.

Quoted 50 lines

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-cmn.c | 16 +++++++++++-----
  1 file changed, 11 insertions(+), 5 deletions(-)

drivers/perf/arm-cmn.c +11 -5

index 8f7a1a6f8ab7..4d128db2040c 100644
@@ -115,6 +115,7 @@
  /* The DTC node is where the magic happens */
  #define CMN_DT_DTC_CTL			0x0a00
  #define CMN_DT_DTC_CTL_DT_EN		BIT(0)
+#define CMN_DT_DTC_CTL_CG_DISABLE	BIT(10)
 
  /* DTC counters are paired in 64-bit registers on a 16-byte stride. Yuck */
  #define _CMN_DT_CNT_REG(n)		((((n) / 2) * 4 + (n) % 2) * 4)

@@ -1544,9 +1545,12 @@ static void arm_cmn_event_start(struct perf_event *event, int flags)
  	int i;
 
  	if (type == CMN_TYPE_DTC) {
-		i = hw->dtc_idx[0];
-		writeq_relaxed(CMN_CC_INIT, cmn->dtc[i].base + CMN_DT_PMCCNTR);
-		cmn->dtc[i].cc_active = true;
+		struct arm_cmn_dtc *dtc = cmn->dtc + hw->dtc_idx[0];
+
+		writel_relaxed(CMN_DT_DTC_CTL_DT_EN | CMN_DT_DTC_CTL_CG_DISABLE,
+			       dtc->base + CMN_DT_DTC_CTL);
+		writeq_relaxed(CMN_CC_INIT, dtc->base + CMN_DT_PMCCNTR);
+		dtc->cc_active = true;
  	} else if (type == CMN_TYPE_WP) {
  		u64 val = CMN_EVENT_WP_VAL(event);
  		u64 mask = CMN_EVENT_WP_MASK(event);

@@ -1575,8 +1579,10 @@ static void arm_cmn_event_stop(struct perf_event *event, int flags)
  	int i;
 
  	if (type == CMN_TYPE_DTC) {
-		i = hw->dtc_idx[0];
-		cmn->dtc[i].cc_active = false;
+		struct arm_cmn_dtc *dtc = cmn->dtc + hw->dtc_idx[0];
+
+		dtc->cc_active = false;
+		writel_relaxed(CMN_DT_DTC_CTL_DT_EN, dtc->base + CMN_DT_DTC_CTL);
  	} else if (type == CMN_TYPE_WP) {
  		for_each_hw_dn(hw, dn, i) {
  			void __iomem *base = dn->pmu_base + CMN_DTM_OFFSET(hw->dtm_offset);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Annoyingly, we're soon going to have to cope with PMU registers moving about. This will mostly be straightforward, except for the hard-coding of CMN_PMU_OFFSET for the DTC PMU registers. As a first step, refactor those accessors to allow for encapsulating a variable offset without making a big mess all over.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm-cmn.c | 64 ++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

Quoted 10 lines · Robin Murphy

Annoyingly, we're soon going to have to cope with PMU registers moving about. This will mostly be straightforward, except for the hard-coding of CMN_PMU_OFFSET for the DTC PMU registers. As a first step, refactor those accessors to allow for encapsulating a variable offset without making a big mess all over.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-cmn.c | 64 ++++++++++++++++++++++++------------------
  1 file changed, 36 insertions(+), 28 deletions(-)

Aside from a minoe comment below this looks fine to me.

drivers/perf/arm-cmn.c struct arm_cmn_dtm { · line 308

Quoted 6 lines

  struct arm_cmn_dtc {
  	void __iomem *base;

drivers/perf/arm-cmn.c +2 -1

+	void __iomem *pmu_base;
  	int irq;
-	int irq_friend;
+	s8 irq_friend;

Unrelated change?

AFAICT there's no reason for 'irq_friend' to change from 'int' to 's8', and nothing in the commit message explains it.

Otherwise this all looks fine to me.

Mark.

Quoted 23 lines · Mark Rutland

Quoted 10 lines · Robin Murphy

Annoyingly, we're soon going to have to cope with PMU registers moving about. This will mostly be straightforward, except for the hard-coding of CMN_PMU_OFFSET for the DTC PMU registers. As a first step, refactor those accessors to allow for encapsulating a variable offset without making a big mess all over.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-cmn.c | 64 ++++++++++++++++++++++++------------------
  1 file changed, 36 insertions(+), 28 deletions(-)

Aside from a minoe comment below this looks fine to me.

drivers/perf/arm-cmn.c struct arm_cmn_dtm { · line 308

Quoted 6 lines

  struct arm_cmn_dtc {
  	void __iomem *base;

drivers/perf/arm-cmn.c +2 -1

+	void __iomem *pmu_base;
  	int irq;
-	int irq_friend;
+	s8 irq_friend;

Unrelated change?

Quoted 2 lines

AFAICT there's no reason for 'irq_friend' to change from 'int' to 's8', and nothing in the commit message explains it.

Oops, I had meant to note in the commit message that this is a little structure repacking where there was a hole already, to compensate for adding the new member... I shall un-forget that for v2.

Quoted 24 lines · Robin Murphy

Quoted 23 lines · Mark Rutland

Quoted 10 lines · Robin Murphy

Annoyingly, we're soon going to have to cope with PMU registers moving about. This will mostly be straightforward, except for the hard-coding of CMN_PMU_OFFSET for the DTC PMU registers. As a first step, refactor those accessors to allow for encapsulating a variable offset without making a big mess all over.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-cmn.c | 64 ++++++++++++++++++++++++------------------
  1 file changed, 36 insertions(+), 28 deletions(-)

Aside from a minoe comment below this looks fine to me.

drivers/perf/arm-cmn.c struct arm_cmn_dtm { · line 308

Quoted 6 lines

  struct arm_cmn_dtc {
  	void __iomem *base;

drivers/perf/arm-cmn.c +2 -1

+	void __iomem *pmu_base;
  	int irq;
-	int irq_friend;
+	s8 irq_friend;

Unrelated change?

Quoted 6 lines

Quoted 2 lines

AFAICT there's no reason for 'irq_friend' to change from 'int' to 's8', and nothing in the commit message explains it.

Oops, I had meant to note in the commit message that this is a little structure repacking where there was a hole already, to compensate for adding the new member... I shall un-forget that for v2.

Cool, with that:

Acked-by Mark Rutland <[email protected]>

Mark.

The CMN S3 PMU is functionally still very similar to CMN-700, however while the register contents are compatible, many of them are moved to different offsets. While this is technically discoverable by a careful driver that understands the part number in the peripheral ID registers (which do at least remain in the same place), a new unique compatible seems warranted to avoid any surprises.

CC <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 Documentation/devicetree/bindings/perf/arm,cmn.yaml | 1 +
 1 file changed, 1 insertion(+)

CMN S3 is the latest and greatest evolution for 2024, although most of the new features don't impact the PMU, so from our point of view it ends up looking a lot like CMN-700 r3 still. We have some new device types to ignore, a mildly irritating rearrangement of the register layouts, and a scary new configuration option that makes it potentially unsafe to even walk the full discovery tree, let alone attempt to use the PMU.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm-cmn.c | 117 ++++++++++++++++++++++++++---------------
 1 file changed, 74 insertions(+), 43 deletions(-)

drivers/perf/arm-cmn.c +74 -43

index 12bbb689a1af..0d19163fba5a 100644
@@ -42,24 +42,28 @@
 #define CMN_CFGM_PERIPH_ID_23		0x0010
 #define CMN_CFGM_PID2_REVISION		GENMASK_ULL(7, 4)
 
-#define CMN_CFGM_INFO_GLOBAL		0x900
+#define CMN_CFGM_INFO_GLOBAL		0x0900
 #define CMN_INFO_MULTIPLE_DTM_EN	BIT_ULL(63)
 #define CMN_INFO_RSP_VC_NUM		GENMASK_ULL(53, 52)
 #define CMN_INFO_DAT_VC_NUM		GENMASK_ULL(51, 50)
+#define CMN_INFO_DEVICE_ISO_ENABLE	BIT_ULL(44)
 
-#define CMN_CFGM_INFO_GLOBAL_1		0x908
+#define CMN_CFGM_INFO_GLOBAL_1		0x0908
 #define CMN_INFO_SNP_VC_NUM		GENMASK_ULL(3, 2)
 #define CMN_INFO_REQ_VC_NUM		GENMASK_ULL(1, 0)
 
 /* XPs also have some local topology info which has uses too */
 #define CMN_MXP__CONNECT_INFO(p)	(0x0008 + 8 * (p))
-#define CMN__CONNECT_INFO_DEVICE_TYPE	GENMASK_ULL(4, 0)
+#define CMN__CONNECT_INFO_DEVICE_TYPE	GENMASK_ULL(5, 0)
 
 #define CMN_MAX_PORTS			6
 #define CI700_CONNECT_INFO_P2_5_OFFSET	0x10
 
 /* PMU registers occupy the 3rd 4KB page of each node's region */
 #define CMN_PMU_OFFSET			0x2000
+/* ...except when they don't :( */
+#define CMN_S3_DTM_OFFSET		0xa000
+#define CMN_S3_PMU_OFFSET		0xd900
 
 /* For most nodes, this is all there is */
 #define CMN_PMU_EVENT_SEL		0x000

@@ -191,10 +195,11 @@ enum cmn_model {
 	CMN650 = 2,
 	CMN700 = 4,
 	CI700 = 8,
+	CMNS3 = 16,
 	/* ...and then we can use bitmap tricks for commonality */
 	CMN_ANY = -1,
 	NOT_CMN600 = -2,
-	CMN_650ON = CMN650 | CMN700,
+	CMN_650ON = CMN650 | CMN700 | CMNS3,
 };
 
 /* Actual part numbers and revision IDs defined by the hardware */

@@ -203,6 +208,7 @@ enum cmn_part {
 	PART_CMN650 = 0x436,
 	PART_CMN700 = 0x43c,
 	PART_CI700 = 0x43a,
+	PART_CMN_S3 = 0x43e,
 };
 
 /* CMN-600 r0px shouldn't exist in silicon, thankfully */

@@ -254,6 +260,7 @@ enum cmn_node_type {
 	CMN_TYPE_HNS = 0x200,
 	CMN_TYPE_HNS_MPAM_S,
 	CMN_TYPE_HNS_MPAM_NS,
+	CMN_TYPE_APB = 0x1000,
 	/* Not a real node type */
 	CMN_TYPE_WP = 0x7770
 };

@@ -404,6 +411,8 @@ static enum cmn_model arm_cmn_model(const struct arm_cmn *cmn)
 		return CMN700;
 	case PART_CI700:
 		return CI700;
+	case PART_CMN_S3:
+		return CMNS3;
 	default:
 		return 0;
 	};

@@ -411,6 +420,11 @@ static enum cmn_model arm_cmn_model(const struct arm_cmn *cmn)
 
 static int arm_cmn_pmu_offset(const struct arm_cmn *cmn, const struct arm_cmn_node *dn)
 {
+	if (cmn->part == PART_CMN_S3) {
+		if (dn->type == CMN_TYPE_XP)
+			return CMN_S3_DTM_OFFSET;
+		return CMN_S3_PMU_OFFSET;
+	}
 	return CMN_PMU_OFFSET;
 }
 

@@ -467,6 +481,9 @@ static const char *arm_cmn_device_type(u8 type)
 		case 0x1c: return "  MTSX  |";
 		case 0x1d: return "  HN-V  |";
 		case 0x1e: return "  CCG   |";
+		case 0x20: return " RN-F_F |";
+		case 0x21: return "RN-F_F_E|";
+		case 0x22: return " SN-F_F |";
 		default:   return "  ????  |";
 	}
 }

@@ -777,8 +794,8 @@ static umode_t arm_cmn_event_attr_is_visible(struct kobject *kobj,
 	CMN_EVENT_ATTR(CMN_ANY, cxha_##_name, CMN_TYPE_CXHA, _event)
 #define CMN_EVENT_CCRA(_name, _event)				\
 	CMN_EVENT_ATTR(CMN_ANY, ccra_##_name, CMN_TYPE_CCRA, _event)
-#define CMN_EVENT_CCHA(_name, _event)				\
-	CMN_EVENT_ATTR(CMN_ANY, ccha_##_name, CMN_TYPE_CCHA, _event)
+#define CMN_EVENT_CCHA(_model, _name, _event)				\
+	CMN_EVENT_ATTR(_model, ccha_##_name, CMN_TYPE_CCHA, _event)
 #define CMN_EVENT_CCLA(_name, _event)				\
 	CMN_EVENT_ATTR(CMN_ANY, ccla_##_name, CMN_TYPE_CCLA, _event)
 #define CMN_EVENT_CCLA_RNI(_name, _event)				\

@@ -1136,42 +1153,43 @@ static struct attribute *arm_cmn_event_attrs[] = {
 	CMN_EVENT_CCRA(wdb_alloc,			0x59),
 	CMN_EVENT_CCRA(ssb_alloc,			0x5a),
 
-	CMN_EVENT_CCHA(rddatbyp,			0x61),
-	CMN_EVENT_CCHA(chirsp_up_stall,			0x62),
-	CMN_EVENT_CCHA(chidat_up_stall,			0x63),
-	CMN_EVENT_CCHA(snppcrd_link0_stall,		0x64),
-	CMN_EVENT_CCHA(snppcrd_link1_stall,		0x65),
-	CMN_EVENT_CCHA(snppcrd_link2_stall,		0x66),
-	CMN_EVENT_CCHA(reqtrk_occ,			0x67),
-	CMN_EVENT_CCHA(rdb_occ,				0x68),
-	CMN_EVENT_CCHA(rdbyp_occ,			0x69),
-	CMN_EVENT_CCHA(wdb_occ,				0x6a),
-	CMN_EVENT_CCHA(snptrk_occ,			0x6b),
-	CMN_EVENT_CCHA(sdb_occ,				0x6c),
-	CMN_EVENT_CCHA(snphaz_occ,			0x6d),
-	CMN_EVENT_CCHA(reqtrk_alloc,			0x6e),
-	CMN_EVENT_CCHA(rdb_alloc,			0x6f),
-	CMN_EVENT_CCHA(rdbyp_alloc,			0x70),
-	CMN_EVENT_CCHA(wdb_alloc,			0x71),
-	CMN_EVENT_CCHA(snptrk_alloc,			0x72),
-	CMN_EVENT_CCHA(sdb_alloc,			0x73),
-	CMN_EVENT_CCHA(snphaz_alloc,			0x74),
-	CMN_EVENT_CCHA(pb_rhu_req_occ,			0x75),
-	CMN_EVENT_CCHA(pb_rhu_req_alloc,		0x76),
-	CMN_EVENT_CCHA(pb_rhu_pcie_req_occ,		0x77),
-	CMN_EVENT_CCHA(pb_rhu_pcie_req_alloc,		0x78),
-	CMN_EVENT_CCHA(pb_pcie_wr_req_occ,		0x79),
-	CMN_EVENT_CCHA(pb_pcie_wr_req_alloc,		0x7a),
-	CMN_EVENT_CCHA(pb_pcie_reg_req_occ,		0x7b),
-	CMN_EVENT_CCHA(pb_pcie_reg_req_alloc,		0x7c),
-	CMN_EVENT_CCHA(pb_pcie_rsvd_req_occ,		0x7d),
-	CMN_EVENT_CCHA(pb_pcie_rsvd_req_alloc,		0x7e),
-	CMN_EVENT_CCHA(pb_rhu_dat_occ,			0x7f),
-	CMN_EVENT_CCHA(pb_rhu_dat_alloc,		0x80),
-	CMN_EVENT_CCHA(pb_rhu_pcie_dat_occ,		0x81),
-	CMN_EVENT_CCHA(pb_rhu_pcie_dat_alloc,		0x82),
-	CMN_EVENT_CCHA(pb_pcie_wr_dat_occ,		0x83),
-	CMN_EVENT_CCHA(pb_pcie_wr_dat_alloc,		0x84),
+	CMN_EVENT_CCHA(CMN_ANY, rddatbyp,		0x61),
+	CMN_EVENT_CCHA(CMN_ANY, chirsp_up_stall,	0x62),
+	CMN_EVENT_CCHA(CMN_ANY, chidat_up_stall,	0x63),
+	CMN_EVENT_CCHA(CMN_ANY, snppcrd_link0_stall,	0x64),
+	CMN_EVENT_CCHA(CMN_ANY, snppcrd_link1_stall,	0x65),
+	CMN_EVENT_CCHA(CMN_ANY, snppcrd_link2_stall,	0x66),
+	CMN_EVENT_CCHA(CMN_ANY, reqtrk_occ,		0x67),
+	CMN_EVENT_CCHA(CMN_ANY, rdb_occ,		0x68),
+	CMN_EVENT_CCHA(CMN_ANY, rdbyp_occ,		0x69),
+	CMN_EVENT_CCHA(CMN_ANY, wdb_occ,		0x6a),
+	CMN_EVENT_CCHA(CMN_ANY, snptrk_occ,		0x6b),
+	CMN_EVENT_CCHA(CMN_ANY, sdb_occ,		0x6c),
+	CMN_EVENT_CCHA(CMN_ANY, snphaz_occ,		0x6d),
+	CMN_EVENT_CCHA(CMN_ANY, reqtrk_alloc,		0x6e),
+	CMN_EVENT_CCHA(CMN_ANY, rdb_alloc,		0x6f),
+	CMN_EVENT_CCHA(CMN_ANY, rdbyp_alloc,		0x70),
+	CMN_EVENT_CCHA(CMN_ANY, wdb_alloc,		0x71),
+	CMN_EVENT_CCHA(CMN_ANY, snptrk_alloc,		0x72),
+	CMN_EVENT_CCHA(CMN_ANY, db_alloc,		0x73),
+	CMN_EVENT_CCHA(CMN_ANY, snphaz_alloc,		0x74),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_req_occ,		0x75),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_req_alloc,	0x76),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_pcie_req_occ,	0x77),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_pcie_req_alloc,	0x78),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_wr_req_occ,	0x79),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_wr_req_alloc,	0x7a),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_reg_req_occ,	0x7b),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_reg_req_alloc,	0x7c),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_rsvd_req_occ,	0x7d),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_rsvd_req_alloc,	0x7e),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_dat_occ,		0x7f),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_dat_alloc,	0x80),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_pcie_dat_occ,	0x81),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_pcie_dat_alloc,	0x82),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_wr_dat_occ,	0x83),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_wr_dat_alloc,	0x84),
+	CMN_EVENT_CCHA(CMNS3, chirsp1_up_stall,		0x85),
 
 	CMN_EVENT_CCLA(rx_cxs,				0x21),
 	CMN_EVENT_CCLA(tx_cxs,				0x22),

@@ -1777,7 +1795,8 @@ static int arm_cmn_event_init(struct perf_event *event)
 		/* ...but the DTM may depend on which port we're watching */
 		if (cmn->multi_dtm)
 			hw->dtm_offset = CMN_EVENT_WP_DEV_SEL(event) / 2;
-	} else if (type == CMN_TYPE_XP && cmn->part == PART_CMN700) {
+	} else if (type == CMN_TYPE_XP &&
+		   (cmn->part == PART_CMN700 || cmn->part == PART_CMN_S3)) {
 		hw->wide_sel = true;
 	}
 

@@ -2264,7 +2283,17 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 	reg = readl_relaxed(cfg_region + CMN_CFGM_PERIPH_ID_23);
 	cmn->rev = FIELD_GET(CMN_CFGM_PID2_REVISION, reg);
 
+	/*
+	 * With the device isolation feature, if firmware has neglected to enable
+	 * an XP port then we risk locking up if we try to access anything behind
+	 * it; however we also have no way to tell from Non-Secure whether any
+	 * given port is disabled or not, so the only way to win is not to play...
+	 */
 	reg = readq_relaxed(cfg_region + CMN_CFGM_INFO_GLOBAL);
+	if (reg & CMN_INFO_DEVICE_ISO_ENABLE) {
+		dev_err(cmn->dev, "Device isolation enabled, not continuing due to risk of lockup\n");
+		return -ENODEV;
+	}
 	cmn->multi_dtm = reg & CMN_INFO_MULTIPLE_DTM_EN;
 	cmn->rsp_vc_num = FIELD_GET(CMN_INFO_RSP_VC_NUM, reg);
 	cmn->dat_vc_num = FIELD_GET(CMN_INFO_DAT_VC_NUM, reg);

@@ -2423,6 +2452,7 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 			case CMN_TYPE_CXLA:
 			case CMN_TYPE_HNS_MPAM_S:
 			case CMN_TYPE_HNS_MPAM_NS:
+			case CMN_TYPE_APB:
 				break;
 			/*
 			 * Split "optimised" combination nodes into separate

@@ -2608,6 +2638,7 @@ static const struct of_device_id arm_cmn_of_match[] = {
 	{ .compatible = "arm,cmn-600", .data = (void *)PART_CMN600 },
 	{ .compatible = "arm,cmn-650" },
 	{ .compatible = "arm,cmn-700" },
+	{ .compatible = "arm,cmn-s3" },
 	{ .compatible = "arm,ci-700" },
 	{}
 };

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

CMN S3 is the latest and greatest evolution for 2024, although most of the new features don't impact the PMU, so from our point of view it ends up looking a lot like CMN-700 r3 still. We have some new device types to ignore, a mildly irritating rearrangement of the register layouts, and a scary new configuration option that makes it potentially unsafe to even walk the full discovery tree, let alone attempt to use the PMU.

Signed-off-by Robin Murphy <[email protected]>

---
drivers/perf/arm-cmn.c | 117 ++++++++++++++++++++++++++---------------

1 file changed, 74 insertions(+), 43 deletions(-)

drivers/perf/arm-cmn.c +74 -43

index 12bbb689a1af..0d19163fba5a 100644
@@ -42,24 +42,28 @@
 #define CMN_CFGM_PERIPH_ID_23		0x0010
 #define CMN_CFGM_PID2_REVISION		GENMASK_ULL(7, 4)
 
-#define CMN_CFGM_INFO_GLOBAL		0x900
+#define CMN_CFGM_INFO_GLOBAL		0x0900
 #define CMN_INFO_MULTIPLE_DTM_EN	BIT_ULL(63)
 #define CMN_INFO_RSP_VC_NUM		GENMASK_ULL(53, 52)
 #define CMN_INFO_DAT_VC_NUM		GENMASK_ULL(51, 50)
+#define CMN_INFO_DEVICE_ISO_ENABLE	BIT_ULL(44)
 
-#define CMN_CFGM_INFO_GLOBAL_1		0x908
+#define CMN_CFGM_INFO_GLOBAL_1		0x0908
 #define CMN_INFO_SNP_VC_NUM		GENMASK_ULL(3, 2)
 #define CMN_INFO_REQ_VC_NUM		GENMASK_ULL(1, 0)
 
 /* XPs also have some local topology info which has uses too */
 #define CMN_MXP__CONNECT_INFO(p)	(0x0008 + 8 * (p))
-#define CMN__CONNECT_INFO_DEVICE_TYPE	GENMASK_ULL(4, 0)
+#define CMN__CONNECT_INFO_DEVICE_TYPE	GENMASK_ULL(5, 0)
 
 #define CMN_MAX_PORTS			6
 #define CI700_CONNECT_INFO_P2_5_OFFSET	0x10
 
 /* PMU registers occupy the 3rd 4KB page of each node's region */
 #define CMN_PMU_OFFSET			0x2000
+/* ...except when they don't :( */
+#define CMN_S3_DTM_OFFSET		0xa000
+#define CMN_S3_PMU_OFFSET		0xd900
 
 /* For most nodes, this is all there is */
 #define CMN_PMU_EVENT_SEL		0x000

@@ -191,10 +195,11 @@ enum cmn_model {
 	CMN650 = 2,
 	CMN700 = 4,
 	CI700 = 8,
+	CMNS3 = 16,
 	/* ...and then we can use bitmap tricks for commonality */
 	CMN_ANY = -1,
 	NOT_CMN600 = -2,
-	CMN_650ON = CMN650 | CMN700,
+	CMN_650ON = CMN650 | CMN700 | CMNS3,
 };
 
 /* Actual part numbers and revision IDs defined by the hardware */

@@ -203,6 +208,7 @@ enum cmn_part {
 	PART_CMN650 = 0x436,
 	PART_CMN700 = 0x43c,
 	PART_CI700 = 0x43a,
+	PART_CMN_S3 = 0x43e,
 };
 
 /* CMN-600 r0px shouldn't exist in silicon, thankfully */

@@ -254,6 +260,7 @@ enum cmn_node_type {
 	CMN_TYPE_HNS = 0x200,
 	CMN_TYPE_HNS_MPAM_S,
 	CMN_TYPE_HNS_MPAM_NS,
+	CMN_TYPE_APB = 0x1000,
 	/* Not a real node type */
 	CMN_TYPE_WP = 0x7770
 };

@@ -404,6 +411,8 @@ static enum cmn_model arm_cmn_model(const struct arm_cmn *cmn)
 		return CMN700;
 	case PART_CI700:
 		return CI700;
+	case PART_CMN_S3:
+		return CMNS3;
 	default:
 		return 0;
 	};

@@ -411,6 +420,11 @@ static enum cmn_model arm_cmn_model(const struct arm_cmn *cmn)
 
 static int arm_cmn_pmu_offset(const struct arm_cmn *cmn, const struct arm_cmn_node *dn)
 {
+	if (cmn->part == PART_CMN_S3) {
+		if (dn->type == CMN_TYPE_XP)
+			return CMN_S3_DTM_OFFSET;
+		return CMN_S3_PMU_OFFSET;
+	}
 	return CMN_PMU_OFFSET;
 }
 

@@ -467,6 +481,9 @@ static const char *arm_cmn_device_type(u8 type)
 		case 0x1c: return "  MTSX  |";
 		case 0x1d: return "  HN-V  |";
 		case 0x1e: return "  CCG   |";
+		case 0x20: return " RN-F_F |";
+		case 0x21: return "RN-F_F_E|";
+		case 0x22: return " SN-F_F |";
 		default:   return "  ????  |";
 	}
 }

@@ -777,8 +794,8 @@ static umode_t arm_cmn_event_attr_is_visible(struct kobject *kobj,
 	CMN_EVENT_ATTR(CMN_ANY, cxha_##_name, CMN_TYPE_CXHA, _event)
 #define CMN_EVENT_CCRA(_name, _event)				\
 	CMN_EVENT_ATTR(CMN_ANY, ccra_##_name, CMN_TYPE_CCRA, _event)
-#define CMN_EVENT_CCHA(_name, _event)				\
-	CMN_EVENT_ATTR(CMN_ANY, ccha_##_name, CMN_TYPE_CCHA, _event)
+#define CMN_EVENT_CCHA(_model, _name, _event)				\
+	CMN_EVENT_ATTR(_model, ccha_##_name, CMN_TYPE_CCHA, _event)
 #define CMN_EVENT_CCLA(_name, _event)				\
 	CMN_EVENT_ATTR(CMN_ANY, ccla_##_name, CMN_TYPE_CCLA, _event)
 #define CMN_EVENT_CCLA_RNI(_name, _event)				\

@@ -1136,42 +1153,43 @@ static struct attribute *arm_cmn_event_attrs[] = {
 	CMN_EVENT_CCRA(wdb_alloc,			0x59),
 	CMN_EVENT_CCRA(ssb_alloc,			0x5a),
 
-	CMN_EVENT_CCHA(rddatbyp,			0x61),
-	CMN_EVENT_CCHA(chirsp_up_stall,			0x62),
-	CMN_EVENT_CCHA(chidat_up_stall,			0x63),
-	CMN_EVENT_CCHA(snppcrd_link0_stall,		0x64),
-	CMN_EVENT_CCHA(snppcrd_link1_stall,		0x65),
-	CMN_EVENT_CCHA(snppcrd_link2_stall,		0x66),
-	CMN_EVENT_CCHA(reqtrk_occ,			0x67),
-	CMN_EVENT_CCHA(rdb_occ,				0x68),
-	CMN_EVENT_CCHA(rdbyp_occ,			0x69),
-	CMN_EVENT_CCHA(wdb_occ,				0x6a),
-	CMN_EVENT_CCHA(snptrk_occ,			0x6b),
-	CMN_EVENT_CCHA(sdb_occ,				0x6c),
-	CMN_EVENT_CCHA(snphaz_occ,			0x6d),
-	CMN_EVENT_CCHA(reqtrk_alloc,			0x6e),
-	CMN_EVENT_CCHA(rdb_alloc,			0x6f),
-	CMN_EVENT_CCHA(rdbyp_alloc,			0x70),
-	CMN_EVENT_CCHA(wdb_alloc,			0x71),
-	CMN_EVENT_CCHA(snptrk_alloc,			0x72),
-	CMN_EVENT_CCHA(sdb_alloc,			0x73),
-	CMN_EVENT_CCHA(snphaz_alloc,			0x74),
-	CMN_EVENT_CCHA(pb_rhu_req_occ,			0x75),
-	CMN_EVENT_CCHA(pb_rhu_req_alloc,		0x76),
-	CMN_EVENT_CCHA(pb_rhu_pcie_req_occ,		0x77),
-	CMN_EVENT_CCHA(pb_rhu_pcie_req_alloc,		0x78),
-	CMN_EVENT_CCHA(pb_pcie_wr_req_occ,		0x79),
-	CMN_EVENT_CCHA(pb_pcie_wr_req_alloc,		0x7a),
-	CMN_EVENT_CCHA(pb_pcie_reg_req_occ,		0x7b),
-	CMN_EVENT_CCHA(pb_pcie_reg_req_alloc,		0x7c),
-	CMN_EVENT_CCHA(pb_pcie_rsvd_req_occ,		0x7d),
-	CMN_EVENT_CCHA(pb_pcie_rsvd_req_alloc,		0x7e),
-	CMN_EVENT_CCHA(pb_rhu_dat_occ,			0x7f),
-	CMN_EVENT_CCHA(pb_rhu_dat_alloc,		0x80),
-	CMN_EVENT_CCHA(pb_rhu_pcie_dat_occ,		0x81),
-	CMN_EVENT_CCHA(pb_rhu_pcie_dat_alloc,		0x82),
-	CMN_EVENT_CCHA(pb_pcie_wr_dat_occ,		0x83),
-	CMN_EVENT_CCHA(pb_pcie_wr_dat_alloc,		0x84),
+	CMN_EVENT_CCHA(CMN_ANY, rddatbyp,		0x61),
+	CMN_EVENT_CCHA(CMN_ANY, chirsp_up_stall,	0x62),
+	CMN_EVENT_CCHA(CMN_ANY, chidat_up_stall,	0x63),
+	CMN_EVENT_CCHA(CMN_ANY, snppcrd_link0_stall,	0x64),
+	CMN_EVENT_CCHA(CMN_ANY, snppcrd_link1_stall,	0x65),
+	CMN_EVENT_CCHA(CMN_ANY, snppcrd_link2_stall,	0x66),
+	CMN_EVENT_CCHA(CMN_ANY, reqtrk_occ,		0x67),
+	CMN_EVENT_CCHA(CMN_ANY, rdb_occ,		0x68),
+	CMN_EVENT_CCHA(CMN_ANY, rdbyp_occ,		0x69),
+	CMN_EVENT_CCHA(CMN_ANY, wdb_occ,		0x6a),
+	CMN_EVENT_CCHA(CMN_ANY, snptrk_occ,		0x6b),
+	CMN_EVENT_CCHA(CMN_ANY, sdb_occ,		0x6c),
+	CMN_EVENT_CCHA(CMN_ANY, snphaz_occ,		0x6d),
+	CMN_EVENT_CCHA(CMN_ANY, reqtrk_alloc,		0x6e),
+	CMN_EVENT_CCHA(CMN_ANY, rdb_alloc,		0x6f),
+	CMN_EVENT_CCHA(CMN_ANY, rdbyp_alloc,		0x70),
+	CMN_EVENT_CCHA(CMN_ANY, wdb_alloc,		0x71),
+	CMN_EVENT_CCHA(CMN_ANY, snptrk_alloc,		0x72),
+	CMN_EVENT_CCHA(CMN_ANY, db_alloc,		0x73),
+	CMN_EVENT_CCHA(CMN_ANY, snphaz_alloc,		0x74),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_req_occ,		0x75),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_req_alloc,	0x76),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_pcie_req_occ,	0x77),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_pcie_req_alloc,	0x78),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_wr_req_occ,	0x79),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_wr_req_alloc,	0x7a),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_reg_req_occ,	0x7b),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_reg_req_alloc,	0x7c),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_rsvd_req_occ,	0x7d),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_rsvd_req_alloc,	0x7e),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_dat_occ,		0x7f),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_dat_alloc,	0x80),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_pcie_dat_occ,	0x81),
+	CMN_EVENT_CCHA(CMN_ANY, pb_rhu_pcie_dat_alloc,	0x82),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_wr_dat_occ,	0x83),
+	CMN_EVENT_CCHA(CMN_ANY, pb_pcie_wr_dat_alloc,	0x84),
+	CMN_EVENT_CCHA(CMNS3, chirsp1_up_stall,		0x85),
 
 	CMN_EVENT_CCLA(rx_cxs,				0x21),
 	CMN_EVENT_CCLA(tx_cxs,				0x22),

@@ -1777,7 +1795,8 @@ static int arm_cmn_event_init(struct perf_event *event)
 		/* ...but the DTM may depend on which port we're watching */
 		if (cmn->multi_dtm)
 			hw->dtm_offset = CMN_EVENT_WP_DEV_SEL(event) / 2;
-	} else if (type == CMN_TYPE_XP && cmn->part == PART_CMN700) {
+	} else if (type == CMN_TYPE_XP &&
+		   (cmn->part == PART_CMN700 || cmn->part == PART_CMN_S3)) {
 		hw->wide_sel = true;
 	}
 

@@ -2264,7 +2283,17 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 	reg = readl_relaxed(cfg_region + CMN_CFGM_PERIPH_ID_23);
 	cmn->rev = FIELD_GET(CMN_CFGM_PID2_REVISION, reg);
 
+	/*
+	 * With the device isolation feature, if firmware has neglected to enable
+	 * an XP port then we risk locking up if we try to access anything behind
+	 * it; however we also have no way to tell from Non-Secure whether any
+	 * given port is disabled or not, so the only way to win is not to play...
+	 */
 	reg = readq_relaxed(cfg_region + CMN_CFGM_INFO_GLOBAL);
+	if (reg & CMN_INFO_DEVICE_ISO_ENABLE) {
+		dev_err(cmn->dev, "Device isolation enabled, not continuing due to risk of lockup\n");
+		return -ENODEV;
+	}
 	cmn->multi_dtm = reg & CMN_INFO_MULTIPLE_DTM_EN;
 	cmn->rsp_vc_num = FIELD_GET(CMN_INFO_RSP_VC_NUM, reg);
 	cmn->dat_vc_num = FIELD_GET(CMN_INFO_DAT_VC_NUM, reg);

@@ -2423,6 +2452,7 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 			case CMN_TYPE_CXLA:
 			case CMN_TYPE_HNS_MPAM_S:
 			case CMN_TYPE_HNS_MPAM_NS:
+			case CMN_TYPE_APB:
 				break;
 			/*
 			 * Split "optimised" combination nodes into separate

@@ -2608,6 +2638,7 @@ static const struct of_device_id arm_cmn_of_match[] = {
 	{ .compatible = "arm,cmn-600", .data = (void *)PART_CMN600 },
 	{ .compatible = "arm,cmn-650" },
 	{ .compatible = "arm,cmn-700" },
+	{ .compatible = "arm,cmn-s3" },
 	{ .compatible = "arm,ci-700" },
 	{}
 };

Signature · 1 line

2.39.2.101.g768bb238c484.dirty