Message-ID: <57b0e938-31e-dbea-e5cb-c31caab1c59f@os.amperecomputing.com>
Date:   Wed, 14 Jun 2023 01:24:15 -0700 (PDT)
From:   Ilkka Koskinen <ilkka@...amperecomputing.com>
To:     Robin Murphy <robin.murphy@....com>
cc:     will@...nel.org, mark.rutland@....com,
        ilkka@...amperecomputing.com, john.g.garry@...cle.com,
        renyu.zj@...ux.alibaba.com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] perf/arm-cmn: Revamp model detection


On Mon, 12 Jun 2023, Robin Murphy wrote:
> CMN implements a set of CoreSight-format peripheral ID registers which
> in principle we should be able to use to identify the hardware. However
> so far we have avoided trying to use the part number field since the
> TRMs have all described it as "configuration dependent". It turns out,
> though, that this is a quirk of the documentation generation process,
> and in fact the part number should always be a stable well-defined field
> which we can trust.
>
> To that end, revamp our model detection to rely less on ACPI/DT, and
> pave the way towards further using the hardware information as an
> identifier for userspace jevent metrics. This includes renaming the
> revision constants to maximise readability.
>
> Signed-off-by: Robin Murphy <robin.murphy@....com>

Hi Robin,

I quickly tested on two different SoCs and the patchset seemed to
work fine.


Reviewed-and-tested-by: Ilkka Koskinen <ilkka@...amperecomputing.com>



> ---
> drivers/perf/arm-cmn.c | 145 ++++++++++++++++++++++++++---------------
> 1 file changed, 93 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 51c703759d3d..8cf4ed932950 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -44,8 +44,11 @@
> #define CMN_MAX_DTMS			(CMN_MAX_XPS + (CMN_MAX_DIMENSION - 1) * 4)
>
> /* The CFG node has various info besides the discovery tree */
> -#define CMN_CFGM_PERIPH_ID_2		0x0010
> -#define CMN_CFGM_PID2_REVISION		GENMASK(7, 4)
> +#define CMN_CFGM_PERIPH_ID_01		0x0008
> +#define CMN_CFGM_PID0_PART_0		GENMASK_ULL(7, 0)
> +#define CMN_CFGM_PID1_PART_1		GENMASK_ULL(35, 32)
> +#define CMN_CFGM_PERIPH_ID_23		0x0010
> +#define CMN_CFGM_PID2_REVISION		GENMASK_ULL(7, 4)
>
> #define CMN_CFGM_INFO_GLOBAL		0x900
> #define CMN_INFO_MULTIPLE_DTM_EN	BIT_ULL(63)
> @@ -186,6 +189,7 @@
> #define CMN_WP_DOWN			2
>
>
> +/* Internal values for encoding event support */
> enum cmn_model {
> 	CMN600 = 1,
> 	CMN650 = 2,
> @@ -197,26 +201,34 @@ enum cmn_model {
> 	CMN_650ON = CMN650 | CMN700,
> };
>
> +/* Actual part numbers and revision IDs defined by the hardware */
> +enum cmn_part {
> +	PART_CMN600 = 0x434,
> +	PART_CMN650 = 0x436,
> +	PART_CMN700 = 0x43c,
> +	PART_CI700 = 0x43a,
> +};
> +
> /* CMN-600 r0px shouldn't exist in silicon, thankfully */
> enum cmn_revision {
> -	CMN600_R1P0,
> -	CMN600_R1P1,
> -	CMN600_R1P2,
> -	CMN600_R1P3,
> -	CMN600_R2P0,
> -	CMN600_R3P0,
> -	CMN600_R3P1,
> -	CMN650_R0P0 = 0,
> -	CMN650_R1P0,
> -	CMN650_R1P1,
> -	CMN650_R2P0,
> -	CMN650_R1P2,
> -	CMN700_R0P0 = 0,
> -	CMN700_R1P0,
> -	CMN700_R2P0,
> -	CI700_R0P0 = 0,
> -	CI700_R1P0,
> -	CI700_R2P0,
> +	REV_CMN600_R1P0,
> +	REV_CMN600_R1P1,
> +	REV_CMN600_R1P2,
> +	REV_CMN600_R1P3,
> +	REV_CMN600_R2P0,
> +	REV_CMN600_R3P0,
> +	REV_CMN600_R3P1,
> +	REV_CMN650_R0P0 = 0,
> +	REV_CMN650_R1P0,
> +	REV_CMN650_R1P1,
> +	REV_CMN650_R2P0,
> +	REV_CMN650_R1P2,
> +	REV_CMN700_R0P0 = 0,
> +	REV_CMN700_R1P0,
> +	REV_CMN700_R2P0,
> +	REV_CI700_R0P0 = 0,
> +	REV_CI700_R1P0,
> +	REV_CI700_R2P0,
> };
>
> enum cmn_node_type {
> @@ -306,7 +318,7 @@ struct arm_cmn {
> 	unsigned int state;
>
> 	enum cmn_revision rev;
> -	enum cmn_model model;
> +	enum cmn_part part;
> 	u8 mesh_x;
> 	u8 mesh_y;
> 	u16 num_xps;
> @@ -394,19 +406,35 @@ static struct arm_cmn_node *arm_cmn_node(const struct arm_cmn *cmn,
> 	return NULL;
> }
>
> +static enum cmn_model arm_cmn_model(const struct arm_cmn *cmn)
> +{
> +	switch (cmn->part) {
> +	case PART_CMN600:
> +		return CMN600;
> +	case PART_CMN650:
> +		return CMN650;
> +	case PART_CMN700:
> +		return CMN700;
> +	case PART_CI700:
> +		return CI700;
> +	default:
> +		return 0;
> +	};
> +}
> +
> 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))
> +		if (cmn->part == PART_CMN600 || cmn->part == PART_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)
> +		if (cmn->part == PART_CI700)
> 			offset += CI700_CONNECT_INFO_P2_5_OFFSET;
> 	}
>
> @@ -640,7 +668,7 @@ static umode_t arm_cmn_event_attr_is_visible(struct kobject *kobj,
>
> 	eattr = container_of(attr, typeof(*eattr), attr.attr);
>
> -	if (!(eattr->model & cmn->model))
> +	if (!(eattr->model & arm_cmn_model(cmn)))
> 		return 0;
>
> 	type = eattr->type;
> @@ -658,7 +686,7 @@ static umode_t arm_cmn_event_attr_is_visible(struct kobject *kobj,
> 		if ((intf & 4) && !(cmn->ports_used & BIT(intf & 3)))
> 			return 0;
>
> -		if (chan == 4 && cmn->model == CMN600)
> +		if (chan == 4 && cmn->part == PART_CMN600)
> 			return 0;
>
> 		if ((chan == 5 && cmn->rsp_vc_num < 2) ||
> @@ -669,19 +697,19 @@ static umode_t arm_cmn_event_attr_is_visible(struct kobject *kobj,
> 	}
>
> 	/* Revision-specific differences */
> -	if (cmn->model == CMN600) {
> -		if (cmn->rev < CMN600_R1P3) {
> +	if (cmn->part == PART_CMN600) {
> +		if (cmn->rev < REV_CMN600_R1P3) {
> 			if (type == CMN_TYPE_CXRA && eventid > 0x10)
> 				return 0;
> 		}
> -		if (cmn->rev < CMN600_R1P2) {
> +		if (cmn->rev < REV_CMN600_R1P2) {
> 			if (type == CMN_TYPE_HNF && eventid == 0x1b)
> 				return 0;
> 			if (type == CMN_TYPE_CXRA || type == CMN_TYPE_CXHA)
> 				return 0;
> 		}
> -	} else if (cmn->model == CMN650) {
> -		if (cmn->rev < CMN650_R2P0 || cmn->rev == CMN650_R1P2) {
> +	} else if (cmn->part == PART_CMN650) {
> +		if (cmn->rev < REV_CMN650_R2P0 || cmn->rev == REV_CMN650_R1P2) {
> 			if (type == CMN_TYPE_HNF && eventid > 0x22)
> 				return 0;
> 			if (type == CMN_TYPE_SBSX && eventid == 0x17)
> @@ -689,8 +717,8 @@ static umode_t arm_cmn_event_attr_is_visible(struct kobject *kobj,
> 			if (type == CMN_TYPE_RNI && eventid > 0x10)
> 				return 0;
> 		}
> -	} else if (cmn->model == CMN700) {
> -		if (cmn->rev < CMN700_R2P0) {
> +	} else if (cmn->part == PART_CMN700) {
> +		if (cmn->rev < REV_CMN700_R2P0) {
> 			if (type == CMN_TYPE_HNF && eventid > 0x2c)
> 				return 0;
> 			if (type == CMN_TYPE_CCHA && eventid > 0x74)
> @@ -698,7 +726,7 @@ static umode_t arm_cmn_event_attr_is_visible(struct kobject *kobj,
> 			if (type == CMN_TYPE_CCLA && eventid > 0x27)
> 				return 0;
> 		}
> -		if (cmn->rev < CMN700_R1P0) {
> +		if (cmn->rev < REV_CMN700_R1P0) {
> 			if (type == CMN_TYPE_HNF && eventid > 0x2b)
> 				return 0;
> 		}
> @@ -1200,7 +1228,7 @@ static u32 arm_cmn_wp_config(struct perf_event *event)
> 	u32 grp = CMN_EVENT_WP_GRP(event);
> 	u32 exc = CMN_EVENT_WP_EXCLUSIVE(event);
> 	u32 combine = CMN_EVENT_WP_COMBINE(event);
> -	bool is_cmn600 = to_cmn(event->pmu)->model == CMN600;
> +	bool is_cmn600 = to_cmn(event->pmu)->part == PART_CMN600;
>
> 	config = FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_DEV_SEL, dev) |
> 		 FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_CHN_SEL, chn) |
> @@ -1520,14 +1548,14 @@ static int arm_cmn_validate_group(struct arm_cmn *cmn, struct perf_event *event)
> 	return ret;
> }
>
> -static enum cmn_filter_select arm_cmn_filter_sel(enum cmn_model model,
> +static enum cmn_filter_select arm_cmn_filter_sel(const struct arm_cmn *cmn,
> 						 enum cmn_node_type type,
> 						 unsigned int eventid)
> {
> 	struct arm_cmn_event_attr *e;
> -	int i;
> +	enum cmn_model model = arm_cmn_model(cmn);
>
> -	for (i = 0; i < ARRAY_SIZE(arm_cmn_event_attrs) - 1; i++) {
> +	for (int i = 0; i < ARRAY_SIZE(arm_cmn_event_attrs) - 1; i++) {
> 		e = container_of(arm_cmn_event_attrs[i], typeof(*e), attr.attr);
> 		if (e->model & model && e->type == type && e->eventid == eventid)
> 			return e->fsel;
> @@ -1570,12 +1598,12 @@ 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->model == CMN700) {
> +	} else if (type == CMN_TYPE_XP && cmn->part == PART_CMN700) {
> 		hw->wide_sel = true;
> 	}
>
> 	/* This is sufficiently annoying to recalculate, so cache it */
> -	hw->filter_sel = arm_cmn_filter_sel(cmn->model, type, eventid);
> +	hw->filter_sel = arm_cmn_filter_sel(cmn, type, eventid);
>
> 	bynodeid = CMN_EVENT_BYNODEID(event);
> 	nodeid = CMN_EVENT_NODEID(event);
> @@ -2055,6 +2083,7 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
> 	void __iomem *cfg_region;
> 	struct arm_cmn_node cfg, *dn;
> 	struct arm_cmn_dtm *dtm;
> +	enum cmn_part part;
> 	u16 child_count, child_poff;
> 	u32 xp_offset[CMN_MAX_XPS];
> 	u64 reg;
> @@ -2066,7 +2095,19 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
> 		return -ENODEV;
>
> 	cfg_region = cmn->base + rgn_offset;
> -	reg = readl_relaxed(cfg_region + CMN_CFGM_PERIPH_ID_2);
> +
> +	reg = readq_relaxed(cfg_region + CMN_CFGM_PERIPH_ID_01);
> +	part = FIELD_GET(CMN_CFGM_PID0_PART_0, reg);
> +	part |= FIELD_GET(CMN_CFGM_PID1_PART_1, reg) << 8;
> +	if (cmn->part && cmn->part != part)
> +		dev_warn(cmn->dev,
> +			 "Firmware binding mismatch: expected part number 0x%x, found 0x%x\n",
> +			 cmn->part, part);
> +	cmn->part = part;
> +	if (!arm_cmn_model(cmn))
> +		dev_warn(cmn->dev, "Unknown part number: 0x%x\n", part);
> +
> +	reg = readl_relaxed(cfg_region + CMN_CFGM_PERIPH_ID_23);
> 	cmn->rev = FIELD_GET(CMN_CFGM_PID2_REVISION, reg);
>
> 	reg = readq_relaxed(cfg_region + CMN_CFGM_INFO_GLOBAL);
> @@ -2130,7 +2171,7 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
> 		if (xp->id == (1 << 3))
> 			cmn->mesh_x = xp->logid;
>
> -		if (cmn->model == CMN600)
> +		if (cmn->part == PART_CMN600)
> 			xp->dtc = 0xf;
> 		else
> 			xp->dtc = 1 << readl_relaxed(xp_region + CMN_DTM_UNIT_INFO);
> @@ -2251,7 +2292,7 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
> 	if (cmn->num_xps == 1)
> 		dev_warn(cmn->dev, "1x1 config not fully supported, translate XP events manually\n");
>
> -	dev_dbg(cmn->dev, "model %d, periph_id_2 revision %d\n", cmn->model, cmn->rev);
> +	dev_dbg(cmn->dev, "periph_id part 0x%03x revision %d\n", cmn->part, cmn->rev);
> 	reg = cmn->ports_used;
> 	dev_dbg(cmn->dev, "mesh %dx%d, ID width %d, ports %6pbl%s\n",
> 		cmn->mesh_x, cmn->mesh_y, arm_cmn_xyidbits(cmn), &reg,
> @@ -2306,17 +2347,17 @@ static int arm_cmn_probe(struct platform_device *pdev)
> 		return -ENOMEM;
>
> 	cmn->dev = &pdev->dev;
> -	cmn->model = (unsigned long)device_get_match_data(cmn->dev);
> +	cmn->part = (unsigned long)device_get_match_data(cmn->dev);
> 	platform_set_drvdata(pdev, cmn);
>
> -	if (cmn->model == CMN600 && has_acpi_companion(cmn->dev)) {
> +	if (cmn->part == PART_CMN600 && has_acpi_companion(cmn->dev)) {
> 		rootnode = arm_cmn600_acpi_probe(pdev, cmn);
> 	} else {
> 		rootnode = 0;
> 		cmn->base = devm_platform_ioremap_resource(pdev, 0);
> 		if (IS_ERR(cmn->base))
> 			return PTR_ERR(cmn->base);
> -		if (cmn->model == CMN600)
> +		if (cmn->part == PART_CMN600)
> 			rootnode = arm_cmn600_of_probe(pdev->dev.of_node);
> 	}
> 	if (rootnode < 0)
> @@ -2404,10 +2445,10 @@ static int arm_cmn_remove(struct platform_device *pdev)
>
> #ifdef CONFIG_OF
> static const struct of_device_id arm_cmn_of_match[] = {
> -	{ .compatible = "arm,cmn-600", .data = (void *)CMN600 },
> -	{ .compatible = "arm,cmn-650", .data = (void *)CMN650 },
> -	{ .compatible = "arm,cmn-700", .data = (void *)CMN700 },
> -	{ .compatible = "arm,ci-700", .data = (void *)CI700 },
> +	{ .compatible = "arm,cmn-600", .data = (void *)PART_CMN600 },
> +	{ .compatible = "arm,cmn-650" },
> +	{ .compatible = "arm,cmn-700" },
> +	{ .compatible = "arm,ci-700" },
> 	{}
> };
> MODULE_DEVICE_TABLE(of, arm_cmn_of_match);
> @@ -2415,9 +2456,9 @@ MODULE_DEVICE_TABLE(of, arm_cmn_of_match);
>
> #ifdef CONFIG_ACPI
> static const struct acpi_device_id arm_cmn_acpi_match[] = {
> -	{ "ARMHC600", CMN600 },
> -	{ "ARMHC650", CMN650 },
> -	{ "ARMHC700", CMN700 },
> +	{ "ARMHC600", PART_CMN600 },
> +	{ "ARMHC650" },
> +	{ "ARMHC700" },
> 	{}
> };
> MODULE_DEVICE_TABLE(acpi, arm_cmn_acpi_match);
> -- 
> 2.39.2.101.g768bb238c484.dirty
>
>

Powered by blists - more mailing lists