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

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

[Robin Murphy]

v1: https://lore.kernel.org/linux-arm-kernel/cover.1701793996.git.robin.murphy@arm.com/

Hi all, here's v2, with tweaks to the binding per Rob's comments,
and fixing patch #5 to hopefully not break ACPI this time.

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            | 149 ++++++++++--------
 drivers/perf/arm_cspmu/arm_cspmu.h            |   1 +
 drivers/perf/arm_cspmu/nvidia_cspmu.c         |   6 -
 4 files changed, 123 insertions(+), 72 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 v2 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.

Reviewed-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
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 v2 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.

Reviewed-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
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 v2 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.

Reviewed-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
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 v2 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 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 <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>
---
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

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..0fcc5bb610f3
--- /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 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
+
+  reg-io-width:
+    description: Granularity at which PMU register accesses are single-copy atomic
+    default: 4
+    enum: [4, 8]
+
+
+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

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

[Rob Herring]

On Thu, 14 Dec 2023 16:31:07 +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 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 <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>
> ---
> 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
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/perf/arm,coresight-pmu.yaml:27:111: [warning] line too long (114 > 110 characters) (line-length)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/3c2dd41b585efe44d361f41fcea0181ff2a9c9c5.1702571292.git.robin.murphy@arm.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


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

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

[Rob Herring]

On Thu, 14 Dec 2023 16:31:07 +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 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 <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>
> ---
> 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
> 

With the line wrapping fixed:

Reviewed-by: Rob Herring <robh@kernel.org>


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

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

[Krzysztof Kozlowski]

On 14/12/2023 17:31, Robin Murphy wrote:
> +
> +  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
> +
> +  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

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

Why no example to validate the binding?

Best regards,
Krzysztof


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

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

[Robin Murphy]

On 15/12/2023 9:44 am, Krzysztof Kozlowski wrote:
> On 14/12/2023 17:31, Robin Murphy wrote:
>> +
>> +  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
>> +
>> +  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

Ack, I've fixed that up locally along with the linewrap (since Will's 
already winding down for holidays, I'm assuming I'll resend this for 6.9 
in the new year now).

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

Thanks,
Robin.

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

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

[Krzysztof Kozlowski]

On 15/12/2023 19:39, Robin Murphy wrote:
>>> +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.

The example is used to validate the schema.

Best regards,
Krzysztof


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

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

[Robin Murphy]

On 2023-12-18 7:03 am, Krzysztof Kozlowski wrote:
> On 15/12/2023 19:39, Robin Murphy wrote:
>>>> +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.
> 
> 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...

Thanks,
Robin.

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

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

[Krzysztof Kozlowski]

On 19/12/2023 15:24, Robin Murphy wrote:
> On 2023-12-18 7:03 am, Krzysztof Kozlowski wrote:
>> On 15/12/2023 19:39, Robin Murphy wrote:
>>>>> +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.
>>
>> 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.

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


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

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

[Robin Murphy]

On 2023-12-20 7:30 am, Krzysztof Kozlowski wrote:
> On 19/12/2023 15:24, Robin Murphy wrote:
>> On 2023-12-18 7:03 am, Krzysztof Kozlowski wrote:
>>> On 15/12/2023 19:39, Robin Murphy wrote:
>>>>>> +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.
>>>
>>> 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.

If I say "All cats are orange.", it's certainly meta-valid in terms of 
being a well-constructed English sentence, but is it true? If I then 
show you a picture of Garfield as an example to prove my assertion, does 
that make it any more true?

As long as a definition is self-consistent to begin with, contriving a 
"correct" example *from* it does not and can not prove anything about 
the overall correctness of that definition. However, I guess that the 
subtlety of that initial condition might be where your argument comes 
from - I've been taking it for granted here since I'm sufficiently 
confident that I can write correct schema which means what I intend it 
to mean, but as a reviewer I appreciate that you're not necessarily 
going to make the same assumption, so there's value for you if patches 
include a sanity check to give the bot a chance to weed out stuff that's 
completely broken. I would still hesitate to call that "validation", though.

Thanks,
Robin.

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

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

[PATCH v2 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 property or
match data mechanism.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
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(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index b64de4d800c7..6c76c135a0cf 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>
 
@@ -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,
-- 
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 v2 5/5] perf/arm_cspmu: Add devicetree support

[Ilkka Koskinen]

On Thu, 14 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 property or
> match data mechanism.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Thanks, this version works fine with ACPI.

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


Cheers, Ilkka

> ---
> 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(-)
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index b64de4d800c7..6c76c135a0cf 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>
>
> @@ -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,
> -- 
> 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 v2 5/5] perf/arm_cspmu: Add devicetree support

[Besar Wicaksono]

Hi Robin,

Please see my comment inline.

> -----Original Message-----
> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Thursday, December 14, 2023 10:31 AM
> To: will@kernel.org
> Cc: mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; suzuki.poulose@arm.com;
> ilkka@os.amperecomputing.com; Besar Wicaksono
> <bwicaksono@nvidia.com>; Yifei Wan <YWan@nvidia.com>; Richard Wiley
> <rwiley@nvidia.com>
> 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 <robin.murphy@arm.com>
> ---
> 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(-)
> 
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
> b/drivers/perf/arm_cspmu/arm_cspmu.c
> index b64de4d800c7..6c76c135a0cf 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>
> 
> @@ -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)

I got a segmentation fault due to null pointer access when calling
arm_cspmu_apmt_node in device tree environment. arm_cspmu_apmt_node
doesn't check a null platform data before dereferencing.
Snipet from arm_cspmu_apmt_node:
                return *(struct acpi_apmt_node **)dev_get_platdata(dev);

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

The ret gives -ENOENT after finish iterating "cpus". I think we could return 0
or void since there is a check for empty associated_cpus down the line.

Thanks,
Besar

> +}
> +
>  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,
> --
> 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