Re: [PATCH 1/8] perf/arm-cmn: Refactor node ID handling. Again.

[Mark Rutland <mark.rutland@arm.com>]

Hi Robin,

On Fri, Aug 09, 2024 at 08:15:40PM +0100, Robin Murphy wrote:
> 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.

> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/perf/arm-cmn.c | 94 ++++++++++++++++++------------------------
>  1 file changed, 40 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index c932d9d355cf..fd2122a37f22 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -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;
> -- 
> 2.39.2.101.g768bb238c484.dirty
>