diff mbox series

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

Message ID 3c2dd41b585efe44d361f41fcea0181ff2a9c9c5.1702571292.git.robin.murphy@arm.com (mailing list archive)
State New, archived
Headers show
Series perf/arm_cspmu: Add devicetree support | expand

Commit Message

Robin Murphy Dec. 14, 2023, 4:31 p.m. UTC
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

Comments

Rob Herring (Arm) Dec. 14, 2023, 5:55 p.m. UTC | #1
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.
Rob Herring (Arm) Dec. 14, 2023, 10:22 p.m. UTC | #2
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>
Krzysztof Kozlowski Dec. 15, 2023, 9:44 a.m. UTC | #3
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
Robin Murphy Dec. 15, 2023, 6:39 p.m. UTC | #4
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.
Krzysztof Kozlowski Dec. 18, 2023, 7:03 a.m. UTC | #5
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
Robin Murphy Dec. 19, 2023, 2:24 p.m. UTC | #6
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.
Krzysztof Kozlowski Dec. 20, 2023, 7:30 a.m. UTC | #7
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
Robin Murphy Dec. 20, 2023, 7:23 p.m. UTC | #8
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
>
diff mbox series

Patch

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