[PATCH 0/5] perf/arm_cspmu: Add devicetree support

[PATCH 0/5] perf/arm_cspmu: Add devicetree support

[Robin Murphy]

Hi all,

This series does a bit more cleanup and adds devicetree support for
CoreSight PMUs. The thing for which I originally wrote this has turned
out not to warrant upstream support, but others have also expressed an
interest so here it is.

Thanks,
Robin.


Robin Murphy (5):
  perf/arm_cspmu: Simplify initialisation
  perf/arm_cspmu: Simplify attribute groups
  perf/arm_cspmu: Simplify counter reset
  dt-bindings/perf: Add Arm CoreSight PMU
  perf/arm_cspmu: Add devicetree support

 .../bindings/perf/arm,coresight-pmu.yaml      |  39 +++++
 drivers/perf/arm_cspmu/arm_cspmu.c            | 155 ++++++++++--------
 drivers/perf/arm_cspmu/arm_cspmu.h            |   1 +
 drivers/perf/arm_cspmu/nvidia_cspmu.c         |   6 -
 4 files changed, 126 insertions(+), 75 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml

-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] perf/arm_cspmu: Simplify initialisation

[Robin Murphy]

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 <robin.murphy@arm.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c    | 55 +++++++++++----------------
 drivers/perf/arm_cspmu/nvidia_cspmu.c |  6 ---
 2 files changed, 22 insertions(+), 39 deletions(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index 2cc35dded007..a3347b1287e6 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -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 *
diff --git a/drivers/perf/arm_cspmu/nvidia_cspmu.c b/drivers/perf/arm_cspmu/nvidia_cspmu.c
index 0382b702f092..5b84b701ad62 100644
--- a/drivers/perf/arm_cspmu/nvidia_cspmu.c
+++ b/drivers/perf/arm_cspmu/nvidia_cspmu.c
@@ -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;
 }
 
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] perf/arm_cspmu: Simplify attribute groups

[Robin Murphy]

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 <robin.murphy@arm.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 26 +++++++++-----------------
 drivers/perf/arm_cspmu/arm_cspmu.h |  1 +
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index a3347b1287e6..f7aa2ac5fd88 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -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,
 	};
 
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
index 2fe723555a6b..c9163acfe810 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.h
+++ b/drivers/perf/arm_cspmu/arm_cspmu.h
@@ -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;
 };
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] perf/arm_cspmu: Simplify counter reset

[Robin Murphy]

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 <robin.murphy@arm.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index f7aa2ac5fd88..b64de4d800c7 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -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);
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] dt-bindings/perf: Add Arm CoreSight PMU

[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.

CC: Rob Herring <robh+dt@kernel.org>
CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
CC: Conor Dooley <conor+dt@kernel.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 .../bindings/perf/arm,coresight-pmu.yaml      | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml

diff --git a/Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml b/Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml
new file mode 100644
index 000000000000..12c7b28eee35
--- /dev/null
+++ b/Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml
@@ -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 <robin.murphy@arm.com>
+
+properties:
+  compatible:
+    const: arm,coresight-pmu
+
+  reg:
+    items:
+      - description: Register page 0
+      - description: Register page 1 (if dual-page extension implemented)
+    minItems: 1
+
+  interrupts:
+    items:
+      - description: Overflow interrupt
+
+  cpus:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    minItems: 1
+    description: List of CPUs with which the PMU is associated, if applicable
+
+  arm,64-bit-atomic:
+    type: boolean
+    description: Register accesses are single-copy atomic at doubleword granularity
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] perf/arm_cspmu: Add devicetree support

[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 <robin.murphy@arm.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 71 +++++++++++++++++++++++-------
 1 file changed, 56 insertions(+), 15 deletions(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index b64de4d800c7..80b5fc417ee3 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -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>
 
@@ -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))
+		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,
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] perf/arm_cspmu: Add devicetree support

[Ilkka Koskinen]

Hi Robin,

On Tue, 5 Dec 2023, Robin Murphy wrote:
> 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 <robin.murphy@arm.com>
> ---
> drivers/perf/arm_cspmu/arm_cspmu.c | 71 +++++++++++++++++++++++-------
> 1 file changed, 56 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index b64de4d800c7..80b5fc417ee3 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -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>
>
> @@ -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

> +		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,
> -- 
> 2.39.2.101.g768bb238c484.dirty
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] perf/arm_cspmu: Simplify counter reset

[Ilkka Koskinen]

On Tue, 5 Dec 2023, Robin Murphy wrote:
> 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 <robin.murphy@arm.com>


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

Cheers, Ilkka


> ---
> drivers/perf/arm_cspmu/arm_cspmu.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index f7aa2ac5fd88..b64de4d800c7 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -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);
> -- 
> 2.39.2.101.g768bb238c484.dirty
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] perf/arm_cspmu: Simplify initialisation

[Ilkka Koskinen]

On Tue, 5 Dec 2023, Robin Murphy wrote:
> 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 <robin.murphy@arm.com>

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

Cheers, Ilkka


> ---
> drivers/perf/arm_cspmu/arm_cspmu.c    | 55 +++++++++++----------------
> drivers/perf/arm_cspmu/nvidia_cspmu.c |  6 ---
> 2 files changed, 22 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 2cc35dded007..a3347b1287e6 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -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 *
> diff --git a/drivers/perf/arm_cspmu/nvidia_cspmu.c b/drivers/perf/arm_cspmu/nvidia_cspmu.c
> index 0382b702f092..5b84b701ad62 100644
> --- a/drivers/perf/arm_cspmu/nvidia_cspmu.c
> +++ b/drivers/perf/arm_cspmu/nvidia_cspmu.c
> @@ -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;
> }
>
> -- 
> 2.39.2.101.g768bb238c484.dirty
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] perf/arm_cspmu: Simplify attribute groups

[Ilkka Koskinen]

On Tue, 5 Dec 2023, Robin Murphy wrote:
> 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 <robin.murphy@arm.com>

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

Cheers, Ilkka


> ---
> drivers/perf/arm_cspmu/arm_cspmu.c | 26 +++++++++-----------------
> drivers/perf/arm_cspmu/arm_cspmu.h |  1 +
> 2 files changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index a3347b1287e6..f7aa2ac5fd88 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -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,
> 	};
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
> index 2fe723555a6b..c9163acfe810 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> @@ -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;
> };
> -- 
> 2.39.2.101.g768bb238c484.dirty
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] perf/arm_cspmu: Add devicetree support

[Robin Murphy]

On 2023-12-06 11:43 pm, Ilkka Koskinen wrote:
> 
> Hi Robin,
> 
> On Tue, 5 Dec 2023, Robin Murphy wrote:
>> 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 <robin.murphy@arm.com>
>> ---
>> drivers/perf/arm_cspmu/arm_cspmu.c | 71 +++++++++++++++++++++++-------
>> 1 file changed, 56 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c 
>> b/drivers/perf/arm_cspmu/arm_cspmu.c
>> index b64de4d800c7..80b5fc417ee3 100644
>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>> @@ -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>
>>
>> @@ -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.

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.

> 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.

> 
> Cheers, Ilkka
> 
>> +        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,
>> -- 
>> 2.39.2.101.g768bb238c484.dirty
>>
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] perf/arm_cspmu: Add devicetree support

[Ilkka Koskinen]

[-- Attachment #1: Type: text/plain, Size: 7274 bytes --]


On Thu, 7 Dec 2023, Robin Murphy wrote:
> On 2023-12-06 11:43 pm, Ilkka Koskinen wrote:
>> 
>> Hi Robin,
>> 
>> On Tue, 5 Dec 2023, Robin Murphy wrote:
>>> 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 <robin.murphy@arm.com>
>>> ---
>>> drivers/perf/arm_cspmu/arm_cspmu.c | 71 +++++++++++++++++++++++-------
>>> 1 file changed, 56 insertions(+), 15 deletions(-)
>>> 
>>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c 
>>> b/drivers/perf/arm_cspmu/arm_cspmu.c
>>> index b64de4d800c7..80b5fc417ee3 100644
>>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
>>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>>> @@ -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>
>>> 
>>> @@ -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.
>
> 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.

Ok, no problem. I'm always happy to give them a spin as soon as I find a 
little time.

Cheers, Ilkka


>
>> 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.
>
>> 
>> Cheers, Ilkka
>> 
>>> +        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,
>>> -- 
>>> 2.39.2.101.g768bb238c484.dirty
>>> 
>>> 
>

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] dt-bindings/perf: Add Arm CoreSight PMU

[Rob Herring]

On Tue, Dec 05, 2023 at 04:51:57PM +0000, Robin Murphy wrote:
> 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.

> CC: Rob Herring <robh+dt@kernel.org>
> CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> CC: Conor Dooley <conor+dt@kernel.org>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  .../bindings/perf/arm,coresight-pmu.yaml      | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml b/Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml
> new file mode 100644
> index 000000000000..12c7b28eee35
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml
> @@ -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 <robin.murphy@arm.com>
> +
> +properties:
> +  compatible:
> +    const: arm,coresight-pmu
> +
> +  reg:
> +    items:
> +      - description: Register page 0
> +      - description: Register page 1 (if dual-page extension implemented)
> +    minItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: Overflow interrupt
> +
> +  cpus:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array

Don't need a type. Already defined.

> +    minItems: 1

1 is always the minimum.

> +    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?

> +
> +  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?

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] dt-bindings/perf: Add Arm CoreSight PMU

[Robin Murphy]

On 2023-12-08 7:33 pm, Rob Herring wrote:
> On Tue, Dec 05, 2023 at 04:51:57PM +0000, Robin Murphy wrote:
>> 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.

Yeah, the architecture seems to have aspirations of being able to 
describe the CPU PMU, but the main intent of this binding is to 
accommodate all of the arbitrary MMIO non-CPU things. However that's not 
to say it *couldn't* ever be used for the memory-mapped view of a CPU's 
PMU via its external debug interface, if it is suitably compatible. I 
concur that I'm rather light on description here, but that's mostly 
because the architecture itself isn't prescriptive - it really is pretty 
much just an interface to whatever PMU functionality can be made to fit 
it (and with plenty of imp-def leeway).

>> CC: Rob Herring <robh+dt@kernel.org>
>> CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>> CC: Conor Dooley <conor+dt@kernel.org>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   .../bindings/perf/arm,coresight-pmu.yaml      | 39 +++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml b/Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml
>> new file mode 100644
>> index 000000000000..12c7b28eee35
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml
>> @@ -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 <robin.murphy@arm.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: arm,coresight-pmu
>> +
>> +  reg:
>> +    items:
>> +      - description: Register page 0
>> +      - description: Register page 1 (if dual-page extension implemented)
>> +    minItems: 1
>> +
>> +  interrupts:
>> +    items:
>> +      - description: Overflow interrupt
>> +
>> +  cpus:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> 
> Don't need a type. Already defined.

Ah, I hadn't noticed this was a common property now. Good to know, thanks.

>> +    minItems: 1
> 
> 1 is always the minimum.
> 
>> +    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?

Affinity to one CPU or some subset, for things like tightly coupled 
accelerators or cluster-level things like DSU, would want explicitly 
describing, but for interconnects, memory controllers and random other 
standalone devices usually no meaningful association will exist (either 
they can be considered affine to no CPUs, or to all of them in an 
implicit manner).

>> +
>> +  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.

It may be recommended, but in practice I'm convinced it's going to 
remain the exception. It's mandatory (and thus assumable) for the 64-bit 
programmers model extension, and effectively moot if only 32-bit or 
smaller counters are implemented, so it's really only relevant to the 
in-between case of the standard "32-bit" programmers model with 64-bit 
(or at least >32-bit) counters, but even then I'd bet most folks are 
still going to implement those behind an APB interface that ends up with 
larger accesses split into 32-bit bursts if they're even accepted at 
all. I'm aware of 3 implementations so far; one I'm not sure how wide 
the counters are, while the other two have proven to be 64-bit *without* 
atomic access ;)

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

Oh, indeed I'd forgotten that was a thing - IIRC in common cases like 
UARTs it's used to represent a *minimum* access size, whereas here it 
would represent a maximum, but I imagine that ambiguity may well already 
exist via other bindings, so as long as that's OK I'm happy to go with 
it. As above I would still be inclined to make it default to 4 if 
absent, but permit either 4 or 8 to be specified.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel