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

[Datta, Shubhrajyoti]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
> Sent: Wednesday, October 19, 2022 5:53 PM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta at amd.com>; linux-arm-
> kernel at lists.infradead.org
> Cc: git (AMD-Xilinx) <git at amd.com>; devicetree at vger.kernel.org;
> will at kernel.org; mark.rutland at arm.com; robh+dt at kernel.org;
> krzysztof.kozlowski+dt at linaro.org; michal.simek at xilinx.com
> Subject: Re: [PATCH v1 1/2] dt-bindings: Add the binding doc for xilinx APM
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> 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
Will update.
> 
> >
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta at amd.com>
> > ---
> >
> > Changes in v1:
> 
> This should be then a v2.
Earlier one was an RFC so I had made it v1.

> 
> >  - 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.yam
> > +++ l
> > @@ -0,0 +1,133 @@
> > +# SPDX-License-Identifier: GPL-2.0-or-later OR BSD-2-Clause %YAML 1.2
> > +---
> > +$id:
> >
> +https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fschemas%2Fperf%2Fxlnx%2Caxi-perf-
> monitor.yaml%23&dat
> >
> +a=05%7C01%7Cshubhrajyoti.datta%40amd.com%7C15905dd06b164f7de3d
> 508dab1
> >
> +ccb630%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638017790
> 04335677
> >
> +2%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMz
> IiLCJBTiI
> >
> +6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=zOrK%2FG
> dlP87S%2FTp
> > +XqdnrNSk0PyJgWRJYU4EZHgJtqMA%3D&reserved=0
> > +$schema:
> >
> +https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C01%7Cshubhrajy
> >
> +oti.datta%40amd.com%7C15905dd06b164f7de3d508dab1ccb630%7C3dd89
> 61fe488
> >
> +4e608e11a82d994e183d%7C0%7C0%7C638017790043356772%7CUnknown
> %7CTWFpbGZ
> >
> +sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%
> >
> +3D%7C3000%7C%7C%7C&sdata=Vl1TpXVHyuS6YmnSP%2BKPOO8D5ap
> 0y9jtV52Q9s
> > +%2F1pvQ%3D&reserved=0
> > +
> > +title: Xilinx Axi Performance Monitor
> > +
> > +maintainers:
> > +  - Shubhrajyoti Datta <shubhrajyoti.datta at 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?
> 
The hardware being on FPGA is configurable what capabilities it will have.
Once chosen to have say the trace it will have tracing capabilities else it will not have.

> > +
> > +  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
Similarly whether event logging capabilities will be there in the hardware is configurable.
If enabled then it have event logging capabilities.

> 
> > +
> > +  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.
> 
I tried to address the comments in 
https://lore.kernel.org/linux-arm-kernel/BY5PR12MB4902474D74155E57BF5D7B9C814F9@BY5PR12MB4902.namprd12.prod.outlook.com/

I thought I have implemented all the comments.  Let me know if I missed something.

Thanks and Regards,
Shubhrajyoti
> Thank you.
> 
> Best regards,
> Krzysztof