perf/arm_cspmu: Add devicetree support

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 following a similar design 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]>

---
v2: Use reg-io-width instead of a new property; tweak descriptions

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

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 following a similar design 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]>

---
v2: Use reg-io-width instead of a new property; tweak descriptions

---
 .../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 +10 -0

+
+  reg:
+    items:
+      - description: Register page 0
+      - description: Register page 1, if the PMU implements the dual-page extension
+    minItems: 1
+
+  interrupts:
+    items:
+      - description: Overflow interrupt

Quoted 14 lines · Robin Murphy

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

+
+  reg:
+    items:
+      - description: Register page 0
+      - description: Register page 1, if the PMU implements the dual-page extension
+    minItems: 1
+
+  interrupts:
+    items:
+      - description: Overflow interrupt
+
+  cpus:
+    description: If the PMU is associated with a particular CPU or subset of CPUs, array of phandles to those CPUs
+

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

Quoted 6 lines

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

+  reg-io-width:
+    description: Granularity at which PMU register accesses are single-copy atomic
+    default: 4
+    enum: [4, 8]
+
+

If there is going to be new posting: just one blank line

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

Quoted 5 lines

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

+required:
+  - compatible
+  - reg
+
+additionalProperties: false

Why no example to validate the binding?

Quoted 7 lines

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

Quoted 5 lines

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

+required:
+  - compatible
+  - reg
+
+additionalProperties: false

Why no example to validate the binding?

IMO for such a trivial binding built out of common properties, an equally trivial example isn't going to add any value, since it won't do anything more than re-state the individual property definitions above. In bindings where we have conditional relationships between properties, or complex encodings where a practical example can help explain a definition (e.g. a map/mask pair for a set of input values), then absolutely, an example can add something more to help the author and/or users. But otherwise, the thing I've really grown to like about schema is how thoroughly self-describing the definitions themselves can now be.

Quoted 8 lines · Robin Murphy

Quoted 7 lines

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

Quoted 5 lines

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

+required:
+  - compatible
+  - reg
+
+additionalProperties: false

Why no example to validate the binding?

Quoted 9 lines

IMO for such a trivial binding built out of common properties, an equally trivial example isn't going to add any value, since it won't do anything more than re-state the individual property definitions above. In bindings where we have conditional relationships between properties, or complex encodings where a practical example can help explain a definition (e.g. a map/mask pair for a set of input values), then absolutely, an example can add something more to help the author and/or users. But otherwise, the thing I've really grown to like about schema is how thoroughly self-describing the definitions themselves can now be.

The example is used to validate the schema.

Quoted 19 lines · Krzysztof Kozlowski

Quoted 17 lines · Robin Murphy

Quoted 7 lines

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

Quoted 5 lines

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

+required:
+  - compatible
+  - reg
+
+additionalProperties: false

Why no example to validate the binding?

IMO for such a trivial binding built out of common properties, an equally trivial example isn't going to add any value, since it won't do anything more than re-state the individual property definitions above. In bindings where we have conditional relationships between properties, or complex encodings where a practical example can help explain a definition (e.g. a map/mask pair for a set of input values), then absolutely, an example can add something more to help the author and/or users. But otherwise, the thing I've really grown to like about schema is how thoroughly self-describing the definitions themselves can now be.

Quoted 1 line

The example is used to validate the schema.

Can you clarify what that *means*, though? As far as I can tell from a bit of experimentation, "make dt_bindings_check" picks up errors in the schema definition itself just the same whether an example is present or not. Thus I still fail to understand what else would be validated by me writing an example here, other than my personal ability to comprehend my own binding.

Yes, I'm well aware that back when we were bootstrapping dtschema it was useful to confirm that schemas were written to correctly describe *existing* known-good DT fragments. However with new bindings like this we've already reached the end goal, where we write an authoritative schema first, then the users follow from there. As I alluded to above, there are reasons why I would actually prefer *not* to provide a usage example here - frankly if a user doesn't understand which parts of the architecture their hardware implements, and/or can't figure out how to copy a single compatible string and write a standard reg property, I would much rather they come to me asking how to write a DT entry, than blindly copy-paste a verbatim example into their DTS, then come to me reporting a "bug" with the driver crashing or failing to probe. I'd love to say I have no experience to base that judgement on, but...

Quoted 20 lines · Robin Murphy

Quoted 19 lines · Krzysztof Kozlowski

Quoted 17 lines · Robin Murphy

Quoted 7 lines

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

Quoted 5 lines

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

+required:
+  - compatible
+  - reg
+
+additionalProperties: false

Why no example to validate the binding?

IMO for such a trivial binding built out of common properties, an equally trivial example isn't going to add any value, since it won't do anything more than re-state the individual property definitions above. In bindings where we have conditional relationships between properties, or complex encodings where a practical example can help explain a definition (e.g. a map/mask pair for a set of input values), then absolutely, an example can add something more to help the author and/or users. But otherwise, the thing I've really grown to like about schema is how thoroughly self-describing the definitions themselves can now be.

Quoted 8 lines

Quoted 1 line

The example is used to validate the schema.

Can you clarify what that *means*, though? As far as I can tell from a bit of experimentation, "make dt_bindings_check" picks up errors in the schema definition itself just the same whether an example is present or not. Thus I still fail to understand what else would be validated by me writing an example here, other than my personal ability to comprehend my own binding.

You miss here the part that the actual binding is used to verify the example used. This is something entirely different than validating schema against meta-schema.

Quoted 14 lines

Yes, I'm well aware that back when we were bootstrapping dtschema it was useful to confirm that schemas were written to correctly describe *existing* known-good DT fragments. However with new bindings like this we've already reached the end goal, where we write an authoritative schema first, then the users follow from there. As I alluded to above, there are reasons why I would actually prefer *not* to provide a usage example here - frankly if a user doesn't understand which parts of the architecture their hardware implements, and/or can't figure out how to copy a single compatible string and write a standard reg property, I would much rather they come to me asking how to write a DT entry, than blindly copy-paste a verbatim example into their DTS, then come to me reporting a "bug" with the driver crashing or failing to probe. I'd love to say I have no experience to base that judgement on, but...

Sure, considering the size of the binding the benefits of the example here are rather low.

Best regards,
Krzysztof

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

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

---
v2: Use APMT node to distinguish ACPI; adjust for binding change
---
drivers/perf/arm_cspmu/arm_cspmu.c | 63 ++++++++++++++++++++++++------

1 file changed, 52 insertions(+), 11 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +52 -11

index b64de4d800c7..6c76c135a0cf 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>
 

@@ -310,6 +311,10 @@ static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu)
 
 	dev = cspmu->dev;
 	apmt_node = arm_cspmu_apmt_node(dev);
+	if (!apmt_node)
+		return devm_kasprintf(dev, GFP_KERNEL, PMUNAME "_%u",
+				      atomic_fetch_inc(&pmu_idx[0]));
+
 	pmu_type = apmt_node->type;
 
 	if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) {

@@ -425,7 +430,7 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 	};
 
 	/* Firmware may override implementer/product ID from PMIIDR */
-	if (apmt_node->impl_id)
+	if (apmt_node && apmt_node->impl_id)
 		cspmu->impl.pmiidr = apmt_node->impl_id;
 
 	/* Find implementer specific attribute ops. */

@@ -940,7 +945,14 @@ static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev)
 	platform_set_drvdata(pdev, cspmu);
 
 	apmt_node = arm_cspmu_apmt_node(dev);
-	cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
+	if (apmt_node) {
+		cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
+	} else {
+		u32 width = 0;
+
+		device_property_read_u32(dev, "reg-io-width", &width);
+		cspmu->has_atomic_dword = (width == 8);
+	}
 
 	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, dev_of_node(cspmu->dev), "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 (arm_cspmu_apmt_node(cspmu->dev))
+		ret = arm_cspmu_acpi_get_cpus(cspmu);
+	else if (device_property_present(cspmu->dev, "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

-----Original Message-----
From: Robin Murphy <[email protected]>
Sent: Thursday, December 14, 2023 10:31 AM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Besar Wicaksono <[email protected]>; Yifei Wan <[email protected]>; Richard Wiley <[email protected]>
Subject: [PATCH v2 5/5] perf/arm_cspmu: Add devicetree support

External email: Use caution opening links or attachments

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

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

---
v2: Use APMT node to distinguish ACPI; adjust for binding change

---
 drivers/perf/arm_cspmu/arm_cspmu.c | 63 ++++++++++++++++++++++++-
-----
 1 file changed, 52 insertions(+), 11 deletions(-)

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

index b64de4d800c7..6c76c135a0cf 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 +3 -0

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

        if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) {

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

@@ -425,7 +430,7 @@ static int arm_cspmu_init_impl_ops(struct
 arm_cspmu *cspmu)
        };
 
        /* Firmware may override implementer/product ID from PMIIDR */
-       if (apmt_node->impl_id)
+       if (apmt_node && apmt_node->impl_id)
                cspmu->impl.pmiidr = apmt_node->impl_id;
 

        /* Find implementer specific attribute ops. */

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

@@ -940,7 +945,14 @@ static struct arm_cspmu *arm_cspmu_alloc(struct
 platform_device *pdev)
        platform_set_drvdata(pdev, cspmu);
 
        apmt_node = arm_cspmu_apmt_node(dev);
-       cspmu->has_atomic_dword = apmt_node->flags &
 ACPI_APMT_FLAGS_ATOMIC;
+       if (apmt_node) {
+               cspmu->has_atomic_dword = apmt_node->flags &
 ACPI_APMT_FLAGS_ATOMIC;
+       } else {
+               u32 width = 0;
+
+               device_property_read_u32(dev, "reg-io-width", &width);
+               cspmu->has_atomic_dword = (width == 8);
+       }

        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 +3 -0

@@ -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;

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

+}
+
 static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
 {
-       return arm_cspmu_acpi_get_cpus(cspmu);
+       int ret = 0;
+
+       if (arm_cspmu_apmt_node(cspmu->dev))
+               ret = arm_cspmu_acpi_get_cpus(cspmu);
+       else if (device_property_present(cspmu->dev, "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,
--
2.39.2.101.g768bb238c484.dirty