Re: [PATCH] perf/arm-cmn: Fix port detection for CMN-700

[Robin Murphy]

On 2023-04-03 12:50, Robin Murphy wrote:
> When the "extra device ports" configuration was first added, the
> additional mxp_device_port_connect_info registers were added around the
> existing mxp_mesh_port_connect_info registers. What I missed about
> CMN-700 is that it shuffled them around to remove this discontinuity.
> As such, tweak the definitions and factor out a helper for reading these
> registers so we can deal with this discrepancy easily, which does at
> least allow nicely tidying up the callsites. With this we can then also
> do the nice thing and skip accesses completely rather than relying on
> RES0 behaviour where we know the extra registers aren't defined.
>
> Fixes: 23760a014417 ("perf/arm-cmn: Add CMN-700 support")
> Reported-by: Jing Zhang <renyu.zj@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> ---
>   drivers/perf/arm-cmn.c | 55 ++++++++++++++++++++++--------------------
>   1 file changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index c9689861be3f..367d41c5d983 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -57,14 +57,12 @@
>   #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_P0	0x0008
> -#define CMN_MXP__CONNECT_INFO_P1	0x0010
> -#define CMN_MXP__CONNECT_INFO_P2	0x0028
> -#define CMN_MXP__CONNECT_INFO_P3	0x0030
> -#define CMN_MXP__CONNECT_INFO_P4	0x0038
> -#define CMN_MXP__CONNECT_INFO_P5	0x0040
> +#define CMN_MXP__CONNECT_INFO(p)	(0x0008 + 8 * (p))
>   #define CMN__CONNECT_INFO_DEVICE_TYPE	GENMASK_ULL(4, 0)
>   
> +#define CMN_MAX_PORTS			6
> +#define CI700_CONNECT_INFO_P2_5_OFFSET	0x20

...which of course should be 0x10 for the way the code below ended up, 
sigh. I'll wait for any further review before resending.

Cheers,
Robin.

> +
>   /* PMU registers occupy the 3rd 4KB page of each node's region */
>   #define CMN_PMU_OFFSET			0x2000
>   
> @@ -396,6 +394,25 @@ static struct arm_cmn_node *arm_cmn_node(const struct arm_cmn *cmn,
>   	return NULL;
>   }
>   
> +static u32 arm_cmn_device_connect_info(const struct arm_cmn *cmn,
> +				       const struct arm_cmn_node *xp, int port)
> +{
> +	int offset = CMN_MXP__CONNECT_INFO(port);
> +
> +	if (port >= 2) {
> +		if (cmn->model & (CMN600 | CMN650))
> +			return 0;
> +		/*
> +		 * CI-700 may have extra ports, but still has the
> +		 * mesh_port_connect_info registers in the way.
> +		 */
> +		if (cmn->model == CI700)
> +			offset += CI700_CONNECT_INFO_P2_5_OFFSET;
> +	}
> +
> +	return readl_relaxed(xp->pmu_base - CMN_PMU_OFFSET + offset);
> +}
> +
>   static struct dentry *arm_cmn_debugfs;
>   
>   #ifdef CONFIG_DEBUG_FS
> @@ -469,7 +486,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;
> -		u8 port[6][CMN_MAX_DIMENSION];
> +		u8 port[CMN_MAX_PORTS][CMN_MAX_DIMENSION];
>   
>   		for (x = 0; x < cmn->mesh_x; x++)
>   			seq_puts(s, "--------+");
> @@ -477,14 +494,9 @@ static int arm_cmn_map_show(struct seq_file *s, void *data)
>   		seq_printf(s, "\n%d    |", y);
>   		for (x = 0; x < cmn->mesh_x; x++) {
>   			struct arm_cmn_node *xp = cmn->xps + xp_base + x;
> -			void __iomem *base = xp->pmu_base - CMN_PMU_OFFSET;
>   
> -			port[0][x] = readl_relaxed(base + CMN_MXP__CONNECT_INFO_P0);
> -			port[1][x] = readl_relaxed(base + CMN_MXP__CONNECT_INFO_P1);
> -			port[2][x] = readl_relaxed(base + CMN_MXP__CONNECT_INFO_P2);
> -			port[3][x] = readl_relaxed(base + CMN_MXP__CONNECT_INFO_P3);
> -			port[4][x] = readl_relaxed(base + CMN_MXP__CONNECT_INFO_P4);
> -			port[5][x] = readl_relaxed(base + CMN_MXP__CONNECT_INFO_P5);
> +			for (p = 0; p < CMN_MAX_PORTS; p++)
> +				port[p][x] = arm_cmn_device_connect_info(cmn, xp, p);
>   			seq_printf(s, " XP #%-2d |", xp_base + x);
>   		}
>   
> @@ -2083,18 +2095,9 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
>   		 * from this, since in that case we will see at least one XP
>   		 * with port 2 connected, for the HN-D.
>   		 */
> -		if (readq_relaxed(xp_region + CMN_MXP__CONNECT_INFO_P0))
> -			xp_ports |= BIT(0);
> -		if (readq_relaxed(xp_region + CMN_MXP__CONNECT_INFO_P1))
> -			xp_ports |= BIT(1);
> -		if (readq_relaxed(xp_region + CMN_MXP__CONNECT_INFO_P2))
> -			xp_ports |= BIT(2);
> -		if (readq_relaxed(xp_region + CMN_MXP__CONNECT_INFO_P3))
> -			xp_ports |= BIT(3);
> -		if (readq_relaxed(xp_region + CMN_MXP__CONNECT_INFO_P4))
> -			xp_ports |= BIT(4);
> -		if (readq_relaxed(xp_region + CMN_MXP__CONNECT_INFO_P5))
> -			xp_ports |= BIT(5);
> +		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))
>   			arm_cmn_init_dtm(dtm++, xp, 1);