perf/arm_cspmu: Add devicetree support

It's far simpler for implementations to literally override whichever default ops they want to, by initialising to the default ops first. This saves all the bother of checking what the impl_init_ops call has or hasn't touched. Make the same clear distinction for the PMIIDR override as well, in case we gain more sources for overriding that in future.

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

---
drivers/perf/arm_cspmu/arm_cspmu.c | 55 +++++++++++----------------
drivers/perf/arm_cspmu/nvidia_cspmu.c | 6 ---

2 files changed, 22 insertions(+), 39 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +22 -33

index 2cc35dded007..a3347b1287e6 100644
@@ -100,13 +100,6 @@
 #define ARM_CSPMU_ACTIVE_CPU_MASK		0x0
 #define ARM_CSPMU_ASSOCIATED_CPU_MASK		0x1
 
-/* Check and use default if implementer doesn't provide attribute callback */
-#define CHECK_DEFAULT_IMPL_OPS(ops, callback)			\
-	do {							\
-		if (!ops->callback)				\
-			ops->callback = arm_cspmu_ ## callback;	\
-	} while (0)
-
 /*
 * Maximum poll count for reading counter value using high-low-high sequence.
 */

@@ -408,21 +401,32 @@ static struct arm_cspmu_impl_match *arm_cspmu_impl_match_get(u32 pmiidr)
 	return NULL;
 }
 
+#define DEFAULT_IMPL_OP(name)	.name = arm_cspmu_##name
+
 static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 {
 	int ret = 0;
-	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
 	struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(cspmu->dev);
 	struct arm_cspmu_impl_match *match;
 
-	/*
-	 * Get PMU implementer and product id from APMT node.
-	 * If APMT node doesn't have implementer/product id, try get it
-	 * from PMIIDR.
-	 */
-	cspmu->impl.pmiidr =
-		(apmt_node->impl_id) ? apmt_node->impl_id :
-				       readl(cspmu->base0 + PMIIDR);
+	/* Start with a default PMU implementation */
+	cspmu->impl.module = THIS_MODULE;
+	cspmu->impl.pmiidr = readl(cspmu->base0 + PMIIDR);
+	cspmu->impl.ops = (struct arm_cspmu_impl_ops) {
+		DEFAULT_IMPL_OP(get_event_attrs),
+		DEFAULT_IMPL_OP(get_format_attrs),
+		DEFAULT_IMPL_OP(get_identifier),
+		DEFAULT_IMPL_OP(get_name),
+		DEFAULT_IMPL_OP(is_cycle_counter_event),
+		DEFAULT_IMPL_OP(event_type),
+		DEFAULT_IMPL_OP(event_filter),
+		DEFAULT_IMPL_OP(set_ev_filter),
+		DEFAULT_IMPL_OP(event_attr_is_visible),
+	};
+
+	/* Firmware may override implementer/product ID from PMIIDR */
+	if (apmt_node->impl_id)
+		cspmu->impl.pmiidr = apmt_node->impl_id;
 
 	/* Find implementer specific attribute ops. */
 	match = arm_cspmu_impl_match_get(cspmu->impl.pmiidr);

@@ -450,24 +454,9 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 		}
 
 		mutex_unlock(&arm_cspmu_lock);
+	}
 
-		if (ret)
-			return ret;
-	} else
-		cspmu->impl.module = THIS_MODULE;
-
-	/* Use default callbacks if implementer doesn't provide one. */
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_event_attrs);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_format_attrs);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_identifier);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_name);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, is_cycle_counter_event);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_type);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_filter);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_attr_is_visible);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter);
-
-	return 0;
+	return ret;
 }
 

static struct attribute_group *

drivers/perf/arm_cspmu/nvidia_cspmu.c +0 -6

index 0382b702f092..5b84b701ad62 100644
@@ -388,12 +388,6 @@ static int nv_cspmu_init_ops(struct arm_cspmu *cspmu)
 	impl_ops->get_format_attrs		= nv_cspmu_get_format_attrs;
 	impl_ops->get_name			= nv_cspmu_get_name;
 
-	/* Set others to NULL to use default callback. */
-	impl_ops->event_type			= NULL;
-	impl_ops->event_attr_is_visible		= NULL;
-	impl_ops->get_identifier		= NULL;
-	impl_ops->is_cycle_counter_event	= NULL;
-
 	return 0;
 }
 

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

The attribute group array itself is always the same, so there's no need to allocate it separately. Storing it directly in our instance data saves memory and gives us one less point of failure.

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

---
drivers/perf/arm_cspmu/arm_cspmu.c | 26 +++++++++----------------- drivers/perf/arm_cspmu/arm_cspmu.h | 1 +

2 files changed, 10 insertions(+), 17 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +9 -17

index a3347b1287e6..f7aa2ac5fd88 100644
@@ -501,23 +501,16 @@ arm_cspmu_alloc_format_attr_group(struct arm_cspmu *cspmu)
 	return format_group;
 }
 
-static struct attribute_group **
-arm_cspmu_alloc_attr_group(struct arm_cspmu *cspmu)
+static int arm_cspmu_alloc_attr_groups(struct arm_cspmu *cspmu)
 {
-	struct attribute_group **attr_groups = NULL;
-	struct device *dev = cspmu->dev;
+	const struct attribute_group **attr_groups = cspmu->attr_groups;
 	const struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
 
 	cspmu->identifier = impl_ops->get_identifier(cspmu);
 	cspmu->name = impl_ops->get_name(cspmu);
 
 	if (!cspmu->identifier || !cspmu->name)
-		return NULL;
-
-	attr_groups = devm_kcalloc(dev, 5, sizeof(struct attribute_group *),
-				   GFP_KERNEL);
-	if (!attr_groups)
-		return NULL;
+		return -ENOMEM;
 
 	attr_groups[0] = arm_cspmu_alloc_event_attr_group(cspmu);
 	attr_groups[1] = arm_cspmu_alloc_format_attr_group(cspmu);

@@ -525,9 +518,9 @@ arm_cspmu_alloc_attr_group(struct arm_cspmu *cspmu)
 	attr_groups[3] = &arm_cspmu_cpumask_attr_group;
 
 	if (!attr_groups[0] || !attr_groups[1])
-		return NULL;
+		return -ENOMEM;
 
-	return attr_groups;
+	return 0;
 }
 
 static inline void arm_cspmu_reset_counters(struct arm_cspmu *cspmu)

@@ -1166,11 +1159,10 @@ static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
 static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
 {
 	int ret, capabilities;
-	struct attribute_group **attr_groups;
 
-	attr_groups = arm_cspmu_alloc_attr_group(cspmu);
-	if (!attr_groups)
-		return -ENOMEM;
+	ret = arm_cspmu_alloc_attr_groups(cspmu);
+	if (ret)
+		return ret;
 
 	ret = cpuhp_state_add_instance(arm_cspmu_cpuhp_state,
 				       &cspmu->cpuhp_node);

@@ -1192,7 +1184,7 @@ static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
 		.start		= arm_cspmu_start,
 		.stop		= arm_cspmu_stop,
 		.read		= arm_cspmu_read,
-		.attr_groups	= (const struct attribute_group **)attr_groups,
+		.attr_groups	= cspmu->attr_groups,
 		.capabilities	= capabilities,
 	};
 

drivers/perf/arm_cspmu/arm_cspmu.h +1 -0

index 2fe723555a6b..c9163acfe810 100644
@@ -157,6 +157,7 @@ struct arm_cspmu {
 	int cycle_counter_logical_idx;
 
 	struct arm_cspmu_hw_events hw_events;
+	const struct attribute_group *attr_groups[5];
 
 	struct arm_cspmu_impl impl;
 };

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

arm_cspmu_reset_counters() inherently also stops them since it is writing 0 to PMCR.E, so there should be no need to do that twice. Also tidy up the reset routine itself for consistency with the start and stop routines, and to be clear at first glance that it is simply writing a constant value.

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

---
drivers/perf/arm_cspmu/arm_cspmu.c | 7 +------

1 file changed, 1 insertion(+), 6 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +1 -6

index f7aa2ac5fd88..b64de4d800c7 100644
@@ -525,11 +525,7 @@ static int arm_cspmu_alloc_attr_groups(struct arm_cspmu *cspmu)
 
 static inline void arm_cspmu_reset_counters(struct arm_cspmu *cspmu)
 {
-	u32 pmcr = 0;
-
-	pmcr |= PMCR_P;
-	pmcr |= PMCR_C;
-	writel(pmcr, cspmu->base0 + PMCR);
+	writel(PMCR_C | PMCR_P, cspmu->base0 + PMCR);
 }
 
 static inline void arm_cspmu_start_counters(struct arm_cspmu *cspmu)

@@ -1189,7 +1185,6 @@ static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
 	};
 
 	/* Hardware counter init */
-	arm_cspmu_stop_counters(cspmu);
 	arm_cspmu_reset_counters(cspmu);
 
 	ret = perf_pmu_register(&cspmu->pmu, cspmu->name, -1);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Add a binding for implementations of the Arm CoreSight Performance Monitoring Unit Architecture. Not to be confused with CoreSight debug and trace, the PMU architecture defines a standard MMIO interface for event counters similar to the CPU PMU architecture, where the implementation and most of its features are discoverable through ID registers.

CC Rob Herring <[email protected]>

CC Krzysztof Kozlowski <[email protected]>

CC Conor Dooley <[email protected]>

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

---
 .../bindings/perf/arm,coresight-pmu.yaml      | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml

Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml new file +17 -0

new file mode 100644
index 000000000000..12c7b28eee35
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/perf/arm,coresight-pmu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Arm Coresight Performance Monitoring Unit Architecture
+
+maintainers:
+  - Robin Murphy <[email protected]>
+
+properties:
+  compatible:
+    const: arm,coresight-pmu
+
+  reg:
+    items:

Quoted 6 lines · Robin Murphy

Add a binding for implementations of the Arm CoreSight Performance Monitoring Unit Architecture. Not to be confused with CoreSight debug and trace, the PMU architecture defines a standard MMIO interface for event counters similar to the CPU PMU architecture, where the implementation and most of its features are discoverable through ID registers.

The implementation is separate from the CPU PMU rather than an MMIO view of it. Not really clear in my quick read of the spec.

Quoted 36 lines

CC Rob Herring <[email protected]>

CC Krzysztof Kozlowski <[email protected]>

CC Conor Dooley <[email protected]>

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

---
  .../bindings/perf/arm,coresight-pmu.yaml      | 39 +++++++++++++++++++
  1 file changed, 39 insertions(+)

create mode 100644 Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml

Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml new file +21 -0

new file mode 100644
index 000000000000..12c7b28eee35
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/perf/arm,coresight-pmu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Arm Coresight Performance Monitoring Unit Architecture
+
+maintainers:
+  - Robin Murphy <[email protected]>
+
+properties:
+  compatible:
+    const: arm,coresight-pmu
+
+  reg:
+    items:
+      - description: Register page 0
+      - description: Register page 1 (if dual-page extension implemented)
+    minItems: 1
+

Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml line 27

Quoted 6 lines

Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml new file +6 -0

+  interrupts:
+    items:
+      - description: Overflow interrupt
+
+  cpus:
+    $ref: /schemas/types.yaml#/definitions/phandle-array

Don't need a type. Already defined.

Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml line 28

Quoted 1 line

Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml new file +1 -0

+    minItems: 1

1 is always the minimum.

Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml line 29

Quoted 1 line

Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml new file +1 -0

+    description: List of CPUs with which the PMU is associated, if applicable

When is it applicable? Presumably when it is associated with only a subset of CPUs?

Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml line 33

Quoted 4 lines

Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml new file +4 -0

+
+  arm,64-bit-atomic:
+    type: boolean
+    description: Register accesses are single-copy atomic at doubleword granularity

As this is recommended, shouldn't the property be the inverse.

Maybe the standard 'reg-io-width = <4>' would be sufficient here?

Hook up devicetree probing support. For now let's hope that people implement PMIIDR properly and we don't need an override mechanism.

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

---
drivers/perf/arm_cspmu/arm_cspmu.c | 71 +++++++++++++++++++++++-------

1 file changed, 56 insertions(+), 15 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +1 -0

index b64de4d800c7..80b5fc417ee3 100644
@@ -27,6 +27,7 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/perf_event.h>
 #include <linux/platform_device.h>
 

drivers/perf/arm_cspmu/arm_cspmu.c +54 -15

+		return devm_kasprintf(dev, GFP_KERNEL, PMUNAME "_%u",
+				      atomic_fetch_inc(&pmu_idx[0]));
+
 	apmt_node = arm_cspmu_apmt_node(dev);
 	pmu_type = apmt_node->type;
 

@@ -406,7 +411,6 @@ static struct arm_cspmu_impl_match *arm_cspmu_impl_match_get(u32 pmiidr)
 static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 {
 	int ret = 0;
-	struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(cspmu->dev);
 	struct arm_cspmu_impl_match *match;
 
 	/* Start with a default PMU implementation */

@@ -425,8 +429,12 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 	};
 
 	/* Firmware may override implementer/product ID from PMIIDR */
-	if (apmt_node->impl_id)
-		cspmu->impl.pmiidr = apmt_node->impl_id;
+	if (has_acpi_companion(cspmu->dev)) {
+		struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(cspmu->dev);
+
+		if (apmt_node->impl_id)
+			cspmu->impl.pmiidr = apmt_node->impl_id;
+	}
 
 	/* Find implementer specific attribute ops. */
 	match = arm_cspmu_impl_match_get(cspmu->impl.pmiidr);

@@ -928,7 +936,6 @@ static void arm_cspmu_read(struct perf_event *event)
 
 static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev)
 {
-	struct acpi_apmt_node *apmt_node;
 	struct arm_cspmu *cspmu;
 	struct device *dev = &pdev->dev;
 

@@ -939,8 +946,13 @@ static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev)
 	cspmu->dev = dev;
 	platform_set_drvdata(pdev, cspmu);
 
-	apmt_node = arm_cspmu_apmt_node(dev);
-	cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
+	if (has_acpi_companion(dev)) {
+		struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(dev);
+
+		cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
+	} else {
+		cspmu->has_atomic_dword = device_property_read_bool(dev, "arm,64-bit-atomic");
+	}
 
 	return cspmu;
 }

@@ -1133,11 +1145,6 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
 		}
 	}
 
-	if (cpumask_empty(&cspmu->associated_cpus)) {
-		dev_dbg(cspmu->dev, "No cpu associated with the PMU\n");
-		return -ENODEV;
-	}
-
 	return 0;
 }
 #else

@@ -1147,9 +1154,36 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
 }
 #endif
 
+static int arm_cspmu_of_get_cpus(struct arm_cspmu *cspmu)
+{
+	struct of_phandle_iterator it;
+	int ret, cpu;
+
+	of_for_each_phandle(&it, ret, cspmu->dev->of_node, "cpus", NULL, 0) {
+		cpu = of_cpu_node_to_id(it.node);
+		if (cpu < 0)
+			continue;
+		cpumask_set_cpu(cpu, &cspmu->associated_cpus);
+	}
+	return ret;
+}
+
 static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
 {
-	return arm_cspmu_acpi_get_cpus(cspmu);
+	int ret = 0;
+
+	if (has_acpi_companion(cspmu->dev))
+		ret = arm_cspmu_acpi_get_cpus(cspmu);
+	else if (of_property_present(cspmu->dev->of_node, "cpus"))
+		ret = arm_cspmu_of_get_cpus(cspmu);
+	else
+		cpumask_copy(&cspmu->associated_cpus, cpu_possible_mask);
+
+	if (!ret && cpumask_empty(&cspmu->associated_cpus)) {
+		dev_dbg(cspmu->dev, "No cpu associated with the PMU\n");
+		ret = -ENODEV;
+	}
+	return ret;
 }
 
 static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)

@@ -1246,11 +1280,18 @@ static const struct platform_device_id arm_cspmu_id[] = {
 };
 MODULE_DEVICE_TABLE(platform, arm_cspmu_id);
 
+static const struct of_device_id arm_cspmu_of_match[] = {
+	{ .compatible = "arm,coresight-pmu" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, arm_cspmu_of_match);
+
 static struct platform_driver arm_cspmu_driver = {
 	.driver = {
-			.name = DRVNAME,
-			.suppress_bind_attrs = true,
-		},
+		.name = DRVNAME,
+		.of_match_table = arm_cspmu_of_match,
+		.suppress_bind_attrs = true,
+	},
 	.probe = arm_cspmu_device_probe,
 	.remove = arm_cspmu_device_remove,
 	.id_table = arm_cspmu_id,

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Hi Robin,

Quoted 21 lines · Robin Murphy

Hook up devicetree probing support. For now let's hope that people implement PMIIDR properly and we don't need an override mechanism.

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

---
drivers/perf/arm_cspmu/arm_cspmu.c | 71 +++++++++++++++++++++++-------

1 file changed, 56 insertions(+), 15 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +1 -0

index b64de4d800c7..80b5fc417ee3 100644
@@ -27,6 +27,7 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/perf_event.h>
 #include <linux/platform_device.h>
 

drivers/perf/arm_cspmu/arm_cspmu.c static const char *arm_cspmu_get_name(const · line 314

Quoted 5 lines

drivers/perf/arm_cspmu/arm_cspmu.c +1 -0

@@ -309,6 +310,10 @@ static const char *arm_cspmu_get_name(const 
 struct arm_cspmu *cspmu)
     static atomic_t pmu_idx[ACPI_APMT_NODE_TYPE_COUNT] = { 0 };
 
     dev = cspmu->dev;
+    if (!has_acpi_companion(dev))

Am I missing something since this doesn't work on top of v6.7-rc4?
The problem I see is that has_acpi_companion() calls is_acpi_device_node(), which compares whether

    fwnode->ops == &acpi_device_fwnode_ops;

However, the acpi/apmt code allocates fwnode by calling acpi_alloc_fwnode_static(), which assigns &acpi_static_fwnode_ops to ops.

I wonder though, if is_acpi_device_node() should check the static variant too? :/

Cheers, Ilkka

Quoted 139 lines

drivers/perf/arm_cspmu/arm_cspmu.c +3 -1

+        return devm_kasprintf(dev, GFP_KERNEL, PMUNAME "_%u",
+                      atomic_fetch_inc(&pmu_idx[0]));
+
     apmt_node = arm_cspmu_apmt_node(dev);
     pmu_type = apmt_node->type;

@@ -406,7 +411,6 @@ static struct arm_cspmu_impl_match 
 *arm_cspmu_impl_match_get(u32 pmiidr)
 static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 {
     int ret = 0;
-    struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(cspmu->dev);
     struct arm_cspmu_impl_match *match;
 

    /* Start with a default PMU implementation */

drivers/perf/arm_cspmu/arm_cspmu.c +6 -2

@@ -425,8 +429,12 @@ static int arm_cspmu_init_impl_ops(struct 
 arm_cspmu *cspmu)
     };
 
     /* Firmware may override implementer/product ID from PMIIDR */
-    if (apmt_node->impl_id)
-        cspmu->impl.pmiidr = apmt_node->impl_id;
+    if (has_acpi_companion(cspmu->dev)) {
+        struct acpi_apmt_node *apmt_node = 
 arm_cspmu_apmt_node(cspmu->dev);
+
+        if (apmt_node->impl_id)
+            cspmu->impl.pmiidr = apmt_node->impl_id;
+    }
 

    /* Find implementer specific attribute ops. */ match = arm_cspmu_impl_match_get(cspmu->impl.pmiidr);

drivers/perf/arm_cspmu/arm_cspmu.c +7 -3

@@ -928,7 +936,6 @@ static void arm_cspmu_read(struct perf_event *event)
 
 static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev)
 {
-    struct acpi_apmt_node *apmt_node;
     struct arm_cspmu *cspmu;
     struct device *dev = &pdev->dev;
 

@@ -939,8 +946,13 @@ static struct arm_cspmu *arm_cspmu_alloc(struct 
 platform_device *pdev)
     cspmu->dev = dev;
     platform_set_drvdata(pdev, cspmu);
 
-    apmt_node = arm_cspmu_apmt_node(dev);
-    cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
+    if (has_acpi_companion(dev)) {
+        struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(dev);
+
+        cspmu->has_atomic_dword = apmt_node->flags & 
 ACPI_APMT_FLAGS_ATOMIC;
+    } else {
+        cspmu->has_atomic_dword = device_property_read_bool(dev, 
 "arm,64-bit-atomic");
+    }

    return cspmu;
}

drivers/perf/arm_cspmu/arm_cspmu.c +0 -5

@@ -1133,11 +1145,6 @@ static int arm_cspmu_acpi_get_cpus(struct 
 arm_cspmu *cspmu)
         }
     }
 
-    if (cpumask_empty(&cspmu->associated_cpus)) {
-        dev_dbg(cspmu->dev, "No cpu associated with the PMU\n");
-        return -ENODEV;
-    }
-
     return 0;
 }

#else

drivers/perf/arm_cspmu/arm_cspmu.c +28 -1

@@ -1147,9 +1154,36 @@ static int arm_cspmu_acpi_get_cpus(struct 
 arm_cspmu *cspmu)
 }
 #endif
 
+static int arm_cspmu_of_get_cpus(struct arm_cspmu *cspmu)
+{
+    struct of_phandle_iterator it;
+    int ret, cpu;
+
+    of_for_each_phandle(&it, ret, cspmu->dev->of_node, "cpus", NULL, 
 0) {
+        cpu = of_cpu_node_to_id(it.node);
+        if (cpu < 0)
+            continue;
+        cpumask_set_cpu(cpu, &cspmu->associated_cpus);
+    }
+    return ret;
+}
+
 static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
 {
-    return arm_cspmu_acpi_get_cpus(cspmu);
+    int ret = 0;
+
+    if (has_acpi_companion(cspmu->dev))
+        ret = arm_cspmu_acpi_get_cpus(cspmu);
+    else if (of_property_present(cspmu->dev->of_node, "cpus"))
+        ret = arm_cspmu_of_get_cpus(cspmu);
+    else
+        cpumask_copy(&cspmu->associated_cpus, cpu_possible_mask);
+
+    if (!ret && cpumask_empty(&cspmu->associated_cpus)) {
+        dev_dbg(cspmu->dev, "No cpu associated with the PMU\n");
+        ret = -ENODEV;
+    }
+    return ret;
 }

static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)

drivers/perf/arm_cspmu/arm_cspmu.c +10 -3

@@ -1246,11 +1280,18 @@ static const struct platform_device_id 
 arm_cspmu_id[] = {
 };
 MODULE_DEVICE_TABLE(platform, arm_cspmu_id);
 
+static const struct of_device_id arm_cspmu_of_match[] = {
+    { .compatible = "arm,coresight-pmu" },
+    {}
+};
+MODULE_DEVICE_TABLE(of, arm_cspmu_of_match);
+
 static struct platform_driver arm_cspmu_driver = {
     .driver = {
-            .name = DRVNAME,
-            .suppress_bind_attrs = true,
-        },
+        .name = DRVNAME,
+        .of_match_table = arm_cspmu_of_match,
+        .suppress_bind_attrs = true,
+    },
     .probe = arm_cspmu_device_probe,
     .remove = arm_cspmu_device_remove,

    .id_table = arm_cspmu_id,

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Quoted 34 lines · Ilkka Koskinen

Hi Robin,

Quoted 21 lines · Robin Murphy

Hook up devicetree probing support. For now let's hope that people implement PMIIDR properly and we don't need an override mechanism.

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

---
drivers/perf/arm_cspmu/arm_cspmu.c | 71 +++++++++++++++++++++++-------

1 file changed, 56 insertions(+), 15 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +1 -0

index b64de4d800c7..80b5fc417ee3 100644
@@ -27,6 +27,7 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/perf_event.h>
 #include <linux/platform_device.h>
 

drivers/perf/arm_cspmu/arm_cspmu.c static const char *arm_cspmu_get_name(const struct · line 314

Quoted 5 lines

drivers/perf/arm_cspmu/arm_cspmu.c +1 -0

@@ -309,6 +310,10 @@ static const char *arm_cspmu_get_name(const struct 
 arm_cspmu *cspmu)
     static atomic_t pmu_idx[ACPI_APMT_NODE_TYPE_COUNT] = { 0 };
 
     dev = cspmu->dev;
+    if (!has_acpi_companion(dev))

Am I missing something since this doesn't work on top of v6.7-rc4?
The problem I see is that has_acpi_companion() calls is_acpi_device_node(), which compares whether

Quoted 5 lines

    fwnode->ops == &acpi_device_fwnode_ops;

However, the acpi/apmt code allocates fwnode by calling acpi_alloc_fwnode_static(), which assigns &acpi_static_fwnode_ops to ops.

Ah, I hadn't got as far as considering that has_acpi_companion() might only work for namespace devices, but it makes sense now that you point it out. I should have clarified that I don't have any suitable ACPI system to test this on, and have only been able to verify basic DT probing on an FPGA.

Quoted 2 lines

I wonder though, if is_acpi_device_node() should check the static variant too? :/

Thanks for the tip, I'll look into that and try to come up with something that works for a v2 (at worst there's always the traditional assumption that !dev->of_node implies ACPI)

Cheers,
Robin.

Quoted 141 lines

Cheers, Ilkka

Quoted 138 lines

drivers/perf/arm_cspmu/arm_cspmu.c +3 -1

+        return devm_kasprintf(dev, GFP_KERNEL, PMUNAME "_%u",
+                      atomic_fetch_inc(&pmu_idx[0]));
+
     apmt_node = arm_cspmu_apmt_node(dev);
     pmu_type = apmt_node->type;

@@ -406,7 +411,6 @@ static struct arm_cspmu_impl_match 
 *arm_cspmu_impl_match_get(u32 pmiidr)
 static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 {
     int ret = 0;
-    struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(cspmu->dev);
     struct arm_cspmu_impl_match *match;
 

    /* Start with a default PMU implementation */

drivers/perf/arm_cspmu/arm_cspmu.c +6 -2

@@ -425,8 +429,12 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu 
 *cspmu)
     };
 
     /* Firmware may override implementer/product ID from PMIIDR */
-    if (apmt_node->impl_id)
-        cspmu->impl.pmiidr = apmt_node->impl_id;
+    if (has_acpi_companion(cspmu->dev)) {
+        struct acpi_apmt_node *apmt_node = 
 arm_cspmu_apmt_node(cspmu->dev);
+
+        if (apmt_node->impl_id)
+            cspmu->impl.pmiidr = apmt_node->impl_id;
+    }
 

    /* Find implementer specific attribute ops. */ match = arm_cspmu_impl_match_get(cspmu->impl.pmiidr);

drivers/perf/arm_cspmu/arm_cspmu.c +7 -3

@@ -928,7 +936,6 @@ static void arm_cspmu_read(struct perf_event *event)
 
 static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev)
 {
-    struct acpi_apmt_node *apmt_node;
     struct arm_cspmu *cspmu;
     struct device *dev = &pdev->dev;
 

@@ -939,8 +946,13 @@ static struct arm_cspmu *arm_cspmu_alloc(struct 
 platform_device *pdev)
     cspmu->dev = dev;
     platform_set_drvdata(pdev, cspmu);
 
-    apmt_node = arm_cspmu_apmt_node(dev);
-    cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
+    if (has_acpi_companion(dev)) {
+        struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(dev);
+
+        cspmu->has_atomic_dword = apmt_node->flags & 
 ACPI_APMT_FLAGS_ATOMIC;
+    } else {
+        cspmu->has_atomic_dword = device_property_read_bool(dev, 
 "arm,64-bit-atomic");
+    }

    return cspmu;
}

drivers/perf/arm_cspmu/arm_cspmu.c +0 -5

@@ -1133,11 +1145,6 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu 
 *cspmu)
         }
     }
 
-    if (cpumask_empty(&cspmu->associated_cpus)) {
-        dev_dbg(cspmu->dev, "No cpu associated with the PMU\n");
-        return -ENODEV;
-    }
-
     return 0;
 }

#else

drivers/perf/arm_cspmu/arm_cspmu.c +28 -1

@@ -1147,9 +1154,36 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu 
 *cspmu)
 }
 #endif
 
+static int arm_cspmu_of_get_cpus(struct arm_cspmu *cspmu)
+{
+    struct of_phandle_iterator it;
+    int ret, cpu;
+
+    of_for_each_phandle(&it, ret, cspmu->dev->of_node, "cpus", NULL, 0) {
+        cpu = of_cpu_node_to_id(it.node);
+        if (cpu < 0)
+            continue;
+        cpumask_set_cpu(cpu, &cspmu->associated_cpus);
+    }
+    return ret;
+}
+
 static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
 {
-    return arm_cspmu_acpi_get_cpus(cspmu);
+    int ret = 0;
+
+    if (has_acpi_companion(cspmu->dev))
+        ret = arm_cspmu_acpi_get_cpus(cspmu);
+    else if (of_property_present(cspmu->dev->of_node, "cpus"))
+        ret = arm_cspmu_of_get_cpus(cspmu);
+    else
+        cpumask_copy(&cspmu->associated_cpus, cpu_possible_mask);
+
+    if (!ret && cpumask_empty(&cspmu->associated_cpus)) {
+        dev_dbg(cspmu->dev, "No cpu associated with the PMU\n");
+        ret = -ENODEV;
+    }
+    return ret;
 }
 

static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)

drivers/perf/arm_cspmu/arm_cspmu.c +10 -3

@@ -1246,11 +1280,18 @@ static const struct platform_device_id 
 arm_cspmu_id[] = {
 };
 MODULE_DEVICE_TABLE(platform, arm_cspmu_id);
 
+static const struct of_device_id arm_cspmu_of_match[] = {
+    { .compatible = "arm,coresight-pmu" },
+    {}
+};
+MODULE_DEVICE_TABLE(of, arm_cspmu_of_match);
+
 static struct platform_driver arm_cspmu_driver = {
     .driver = {
-            .name = DRVNAME,
-            .suppress_bind_attrs = true,
-        },
+        .name = DRVNAME,
+        .of_match_table = arm_cspmu_of_match,
+        .suppress_bind_attrs = true,
+    },
     .probe = arm_cspmu_device_probe,
     .remove = arm_cspmu_device_remove,

    .id_table = arm_cspmu_id,

Signature · 1 line

2.39.2.101.g768bb238c484.dirty