Hi all,
Following on from the disussion on Jing's proposal[1], I've had
confirmation that we do actually have a reliable hardware identifier,
so this mini-series does a bit of refactoring to wire that up and then
expose a sysfs identifier based on it.
Thanks,
Robin.
[1] https://lore.kernel.org/r/[email protected]/
Robin Murphy (2):
perf/arm-cmn: Revamp model detection
perf/arm-cmn: Add sysfs identifier
drivers/perf/arm-cmn.c | 165 +++++++++++++++++++++++++++--------------
1 file changed, 109 insertions(+), 56 deletions(-)
--
2.39.2.101.g768bb238c484.dirty
Expose a sysfs identifier encapsulating the CMN part number and revision
so that jevents can narrow down a fundamental set of possible events for
calculating metrics. Configuration-dependent aspects - such as whether a
given node type is present, and/or a given node ID is valid - are still
not covered, and in general it's hard to see how userspace could handle
them, so we won't be removing any data or validation logic from the
driver any time soon, but at least it's a step in a useful direction.
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/perf/arm-cmn.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index 8cf4ed932950..088dc5c690a4 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -1199,19 +1199,31 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev,
static struct device_attribute arm_cmn_cpumask_attr =
__ATTR(cpumask, 0444, arm_cmn_cpumask_show, NULL);
-static struct attribute *arm_cmn_cpumask_attrs[] = {
+static ssize_t arm_cmn_identifier_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev));
+
+ return sysfs_emit(buf, "%03x%02x\n", cmn->part, cmn->rev);
+}
+
+static struct device_attribute arm_cmn_identifier_attr =
+ __ATTR(identifier, 0444, arm_cmn_identifier_show, NULL);
+
+static struct attribute *arm_cmn_other_attrs[] = {
&arm_cmn_cpumask_attr.attr,
+ &arm_cmn_identifier_attr.attr,
NULL,
};
-static const struct attribute_group arm_cmn_cpumask_attr_group = {
- .attrs = arm_cmn_cpumask_attrs,
+static const struct attribute_group arm_cmn_other_attrs_group = {
+ .attrs = arm_cmn_other_attrs,
};
static const struct attribute_group *arm_cmn_attr_groups[] = {
&arm_cmn_event_attrs_group,
&arm_cmn_format_attrs_group,
- &arm_cmn_cpumask_attr_group,
+ &arm_cmn_other_attrs_group,
NULL
};
--
2.39.2.101.g768bb238c484.dirty
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 <[email protected]>
---
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), ®,
@@ -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
On Mon, 12 Jun 2023, Robin Murphy wrote:
> Expose a sysfs identifier encapsulating the CMN part number and revision
> so that jevents can narrow down a fundamental set of possible events for
> calculating metrics. Configuration-dependent aspects - such as whether a
> given node type is present, and/or a given node ID is valid - are still
> not covered, and in general it's hard to see how userspace could handle
> them, so we won't be removing any data or validation logic from the
> driver any time soon, but at least it's a step in a useful direction.
>
> Signed-off-by: Robin Murphy <[email protected]>
Reviewed-and-tested-by: Ilkka Koskinen <[email protected]>
> ---
> drivers/perf/arm-cmn.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 8cf4ed932950..088dc5c690a4 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -1199,19 +1199,31 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev,
> static struct device_attribute arm_cmn_cpumask_attr =
> __ATTR(cpumask, 0444, arm_cmn_cpumask_show, NULL);
>
> -static struct attribute *arm_cmn_cpumask_attrs[] = {
> +static ssize_t arm_cmn_identifier_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev));
> +
> + return sysfs_emit(buf, "%03x%02x\n", cmn->part, cmn->rev);
> +}
> +
> +static struct device_attribute arm_cmn_identifier_attr =
> + __ATTR(identifier, 0444, arm_cmn_identifier_show, NULL);
> +
> +static struct attribute *arm_cmn_other_attrs[] = {
> &arm_cmn_cpumask_attr.attr,
> + &arm_cmn_identifier_attr.attr,
> NULL,
> };
>
> -static const struct attribute_group arm_cmn_cpumask_attr_group = {
> - .attrs = arm_cmn_cpumask_attrs,
> +static const struct attribute_group arm_cmn_other_attrs_group = {
> + .attrs = arm_cmn_other_attrs,
> };
>
> static const struct attribute_group *arm_cmn_attr_groups[] = {
> &arm_cmn_event_attrs_group,
> &arm_cmn_format_attrs_group,
> - &arm_cmn_cpumask_attr_group,
> + &arm_cmn_other_attrs_group,
> NULL
> };
>
> --
> 2.39.2.101.g768bb238c484.dirty
>
>
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 <[email protected]>
Hi Robin,
I quickly tested on two different SoCs and the patchset seemed to
work fine.
Reviewed-and-tested-by: Ilkka Koskinen <[email protected]>
> ---
> 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), ®,
> @@ -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
>
>
在 2023/6/13 上午1:16, Robin Murphy 写道:
> Expose a sysfs identifier encapsulating the CMN part number and revision
> so that jevents can narrow down a fundamental set of possible events for
> calculating metrics. Configuration-dependent aspects - such as whether a
> given node type is present, and/or a given node ID is valid - are still
> not covered, and in general it's hard to see how userspace could handle
> them, so we won't be removing any data or validation logic from the
> driver any time soon, but at least it's a step in a useful direction.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/perf/arm-cmn.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 8cf4ed932950..088dc5c690a4 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -1199,19 +1199,31 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev,
> static struct device_attribute arm_cmn_cpumask_attr =
> __ATTR(cpumask, 0444, arm_cmn_cpumask_show, NULL);
>
> -static struct attribute *arm_cmn_cpumask_attrs[] = {
> +static ssize_t arm_cmn_identifier_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev));
> +
> + return sysfs_emit(buf, "%03x%02x\n", cmn->part, cmn->rev);
> +}
> +
> +static struct device_attribute arm_cmn_identifier_attr =
> + __ATTR(identifier, 0444, arm_cmn_identifier_show, NULL);
> +
> +static struct attribute *arm_cmn_other_attrs[] = {
> &arm_cmn_cpumask_attr.attr,
> + &arm_cmn_identifier_attr.attr,
> NULL,
> };
>
> -static const struct attribute_group arm_cmn_cpumask_attr_group = {
> - .attrs = arm_cmn_cpumask_attrs,
> +static const struct attribute_group arm_cmn_other_attrs_group = {
> + .attrs = arm_cmn_other_attrs,
> };
>
> static const struct attribute_group *arm_cmn_attr_groups[] = {
> &arm_cmn_event_attrs_group,
> &arm_cmn_format_attrs_group,
> - &arm_cmn_cpumask_attr_group,
> + &arm_cmn_other_attrs_group,
> NULL
> };
>
Tested-by: Jing Zhang <[email protected]>
Thanks,
Jing
On 12/06/2023 18:16, Robin Murphy wrote:
> Expose a sysfs identifier encapsulating the CMN part number and revision
> so that jevents can narrow down a fundamental set of possible events for
> calculating metrics. Configuration-dependent aspects - such as whether a
> given node type is present, and/or a given node ID is valid - are still
> not covered, and in general it's hard to see how userspace could handle
> them, so we won't be removing any data or validation logic from the
> driver any time soon, but at least it's a step in a useful direction.
>
> Signed-off-by: Robin Murphy <[email protected]>
FWIW, Reviewed-by:
John Garry <[email protected]>
However a comment, below.
> ---
> drivers/perf/arm-cmn.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 8cf4ed932950..088dc5c690a4 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -1199,19 +1199,31 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev,
> static struct device_attribute arm_cmn_cpumask_attr =
> __ATTR(cpumask, 0444, arm_cmn_cpumask_show, NULL);
>
> -static struct attribute *arm_cmn_cpumask_attrs[] = {
> +static ssize_t arm_cmn_identifier_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev));
> +
> + return sysfs_emit(buf, "%03x%02x\n", cmn->part, cmn->rev);
I don't think that this gives the "0x" prefix, right? It is just an
encoded string of values, so not strictly necessary or even appropriate,
I suppose.
However, if userspace wants to improve scalability by, say, matching an
event for all revs of a model, userspace (perf tool) needs to be
programmed in the JSONs somehow since we have no delimiter.
Thanks,
John
> +}
> +
> +static struct device_attribute arm_cmn_identifier_attr =
> + __ATTR(identifier, 0444, arm_cmn_identifier_show, NULL);
> +
> +static struct attribute *arm_cmn_other_attrs[] = {
> &arm_cmn_cpumask_attr.attr,
> + &arm_cmn_identifier_attr.attr,
> NULL,
> };
>
> -static const struct attribute_group arm_cmn_cpumask_attr_group = {
> - .attrs = arm_cmn_cpumask_attrs,
> +static const struct attribute_group arm_cmn_other_attrs_group = {
> + .attrs = arm_cmn_other_attrs,
> };
>
> static const struct attribute_group *arm_cmn_attr_groups[] = {
> &arm_cmn_event_attrs_group,
> &arm_cmn_format_attrs_group,
> - &arm_cmn_cpumask_attr_group,
> + &arm_cmn_other_attrs_group,
> NULL
> };
>
On 2023-06-14 15:55, John Garry wrote:
> On 12/06/2023 18:16, Robin Murphy wrote:
>> Expose a sysfs identifier encapsulating the CMN part number and revision
>> so that jevents can narrow down a fundamental set of possible events for
>> calculating metrics. Configuration-dependent aspects - such as whether a
>> given node type is present, and/or a given node ID is valid - are still
>> not covered, and in general it's hard to see how userspace could handle
>> them, so we won't be removing any data or validation logic from the
>> driver any time soon, but at least it's a step in a useful direction.
>>
>> Signed-off-by: Robin Murphy <[email protected]>
>
> FWIW, Reviewed-by:
> John Garry <[email protected]>
>
> However a comment, below.
>
>> ---
>> drivers/perf/arm-cmn.c | 20 ++++++++++++++++----
>> 1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
>> index 8cf4ed932950..088dc5c690a4 100644
>> --- a/drivers/perf/arm-cmn.c
>> +++ b/drivers/perf/arm-cmn.c
>> @@ -1199,19 +1199,31 @@ static ssize_t arm_cmn_cpumask_show(struct
>> device *dev,
>> static struct device_attribute arm_cmn_cpumask_attr =
>> __ATTR(cpumask, 0444, arm_cmn_cpumask_show, NULL);
>> -static struct attribute *arm_cmn_cpumask_attrs[] = {
>> +static ssize_t arm_cmn_identifier_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev));
>> +
>> + return sysfs_emit(buf, "%03x%02x\n", cmn->part, cmn->rev);
>
> I don't think that this gives the "0x" prefix, right? It is just an
> encoded string of values, so not strictly necessary or even appropriate,
> I suppose.
Indeed that was deliberate, to emphasise that this is still a string (of
hex digits), not a single numeric value.
> However, if userspace wants to improve scalability by, say, matching an
> event for all revs of a model, userspace (perf tool) needs to be
> programmed in the JSONs somehow since we have no delimiter.
FWIW I don't mind the original idea of prefix-based string matching - it
feels like about the right level of compromise to give a sufficient
degree of expressiveness without having to go as far as inventing some
whole crazy expression syntax for interpreting values - so all I've
really done here vs. Jing's patch is streamline the string itself. I'm
still assuming the same general usage model, such that a hypothetical
JSON encoding of, say, the hnf_snp_sent_cluster event could have:
"Compat": {"43603","43c??"} /* CMN-650 r2p0, all CMN-700 */
(array and explicit wildcard syntax made up for the sake of implied
debate - not sure if I have a particular preference either way)
If we assume that over time, events are more likely to be added and
stick around than to be removed, then what might be handy would be the
additional notion of something like a "CompatExcept" property to
describe negative matches. That could definitely scale better for the
NOT_CMN600 and CMN_650ON cases in the current driver logic. Then what
would also be really cool would be the ability to define events
hierarchically based on other aliases - e.g. the driver could just
expose a general "sbsx" sysfs alias as "type=5,eventid=?" if SBSX nodes
are present, then the detailed events like "sbsx_rd_req" are somehow
encoded in JSON as the equivalent of "sbsx,eventid=1" such that they're
hidden if not relevant - at which point I think I could actually punt
all the fiddly bits out of driver code into JSON :D
Cheers,
Robin.
On Mon, 12 Jun 2023 18:16:31 +0100, Robin Murphy wrote:
> Following on from the disussion on Jing's proposal[1], I've had
> confirmation that we do actually have a reliable hardware identifier,
> so this mini-series does a bit of refactoring to wire that up and then
> expose a sysfs identifier based on it.
>
> Thanks,
> Robin.
>
> [...]
Applied to will (for-next/perf), thanks!
[1/2] perf/arm-cmn: Revamp model detection
https://git.kernel.org/will/c/7819e05a0dce
[2/2] perf/arm-cmn: Add sysfs identifier
https://git.kernel.org/will/c/a1c45d3ebd30
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev