Re: [PATCH v1 1/2] dt-bindings: Add the binding doc for xilinx APM

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 19/10/2022 05:17, Shubhrajyoti Datta wrote:
> The LogiCORE IP AXI Performance Monitor core enables AXI system
> performance measurement for multiple slots (AXI4/AXI3/
> AXI4-Stream/AXI4-Lite) activity. Add the devicetree binding for
> xilinx APM.

Subject:
Drop redundant "bindings" and add missing prefix, so:

dt-bindings: perf: Add Xilinx APM

> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> 
> Changes in v1:

This should be then a v2.

>  - Use boolean for the values xlnx,enable-profile , xlnx,enable-trace
> and xlnx,enable-event-count
> - Update the file name
> - use generic node name pmu
> 
>  .../bindings/perf/xlnx,axi-perf-monitor.yaml  | 133 ++++++++++++++++++
>  1 file changed, 133 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/perf/xlnx,axi-perf-monitor.yaml
> 
> diff --git a/Documentation/devicetree/bindings/perf/xlnx,axi-perf-monitor.yaml b/Documentation/devicetree/bindings/perf/xlnx,axi-perf-monitor.yaml
> new file mode 100644
> index 000000000000..bd3a9dbc1184
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/perf/xlnx,axi-perf-monitor.yaml
> @@ -0,0 +1,133 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/perf/xlnx,axi-perf-monitor.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx Axi Performance Monitor
> +
> +maintainers:
> +  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> +
> +properties:
> +  compatible:
> +    const: xlnx,axi-perf-monitor
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  xlnx,enable-profile:
> +    description:
> +      Enables the profile mode. Event counting in profile mode consists of a
> +      fixed number of accumulators for each AXI4/AXI3/AXI4-Lite slot. All the
> +      events that can be counted are detected and given to the accumulator
> +      which calculates the aggregate value. There is no selection of events,
> +      and in this mode, event counting is done only on AXI4/AXI3/AXI4-Lite
> +      monitor slots.
> +    type: boolean
> +
> +  xlnx,enable-trace:
> +    description:
> +      Enables trace mode. In trace mode, the APM provides event logging in a
> +      reduced dynamic configuration. It captures the specified AXI events,
> +      external events and the time stamp difference between two successive
> +      events into the streaming FIFO. The selection of events to be captured
> +      is set through parameter configuration. Streaming agents are not
> +      supported in trace mode.
> +    type: boolean

Both enable profile and enable trace do not look like hardware
properties, but rather runtime features. In what use case this enabling
trace or profile should be a property of a hardware?

> +
> +  xlnx,num-monitor-slots:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Number of monitor slots.
> +    minimum: 1
> +    maximum: 8
> +
> +  xlnx,enable-event-count:
> +    description:
> +      Enable event count.

The same

> +    type: boolean
> +
> +  xlnx,enable-event-log:
> +    type: boolean
> +    description:
> +      Enable event log.

The same

> +
> +  xlnx,have-sampled-metric-cnt:
> +    type: boolean
> +    description:
> +      Sampled metric counters enabled in APM.
> +
> +  xlnx,metric-count-width:
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum: [32, 64]

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

Best regards,
Krzysztof