Re: [RFC PATCH v1 2/2] perf/smmuv3: To support the dts to get options

[Rob Herring <robh@kernel.org>]

On Tue, Jul 07, 2020 at 05:01:14PM +0200, Jean-Philippe Brucker wrote:
> Hi,
> 
> On Mon, Jul 06, 2020 at 04:03:34PM +0100, Robin Murphy wrote:
> > On 2020-07-06 12:22, Jay Chen wrote:
> > > For the smmuv3 pmu for support the dts to get the
> > > options
> > > 
> > > Signed-off-by: Jay Chen <jkchen@linux.alibaba.com>
> [...]
> > > +static const struct of_device_id smmu_pmu_of_match[] = {
> > > +	{ .compatible = "arm-smmu-v3-pmcg", },
> > 
> > Please define the DT binding first. IIRC Jean-Philippe wrote some patches a
> > while back that never got posted, but I suppose it should be YAML now...
> 
> Yes, I've never followed through with that because it only supported the
> RevC FastModel with non-default model parameters. I attached the binding I
> currently have, converted to YAML.
> 
> Thanks,
> Jean
> 

> >From b117e5b4ce96a5a8327333ab408cf61200850d4f Mon Sep 17 00:00:00 2001
> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Date: Tue, 7 Jul 2020 16:55:16 +0200
> Subject: [PATCH] dt-bindings: Add SMMUv3 PMCG binding
> 
> Add binding for the SMMUv3 PMU. Each node represents a PMCG, and is placed
> as a sibling node of the SMMU. As PMCGs are mainly implementation
> defined there is no 1-1 relation between SMMU and PMCG. The SMMU could
> have PMU counters for the TCU and each TBU, or a single PMCG.
> 
> TODO: although the Linux implementation doesn't need them, it'd be nice
> to have links from the PMCG node to its associated SMMU. IORT does offer
> this (Node reference) and perhaps it could later help users figure out
> which PMCG is which on systems with dozens of SMMU.

Is the PMCG really a separate block or a new node is just convenient to 
instantiate a driver?

> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  .../bindings/iommu/arm,smmu-v3-pmcg.yaml      | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml
> new file mode 100644
> index 000000000000..23190a617e7e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: GPL-2.0-only

Dual license new bindings.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iommu/arm,smmu-v3-pmcg.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM SMMUv3 Performance Monitor Counter Group
> +
> +maintainers:
> +  - Will Deacon <will@kernel.org>
> +  - Robin Murphy <Robin.Murphy@arm.com>
> +
> +description: |+
> +  An SMMUv3 may have several Performance Monitor Counter Group (PMCG).
> +  They are standalone performance monitoring units that support both
> +  architected and IMPLEMENTATION DEFINED event counters.
> +
> +properties:
> +  $nodename:
> +    pattern: "^smmu-pmcg@[0-9a-f]*"

Should be generic:

pmu@...

(or whatever we've used for PMUs).

> +  compatible:
> +    const: arm,smmu-v3-pmcg

This is correct, but doesn't match the driver.

> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2

More than 1 entry needs a description of what each one is.

A variable number of 'reg' entries generally implies more than 1 
compatible unless the 2nd entry is optional.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  msi-parent: true
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    tcu: smmu-pmcg@2b420000 {

Drop unused labels.

> +            compatible = "arm,smmu-v3-pmcg";
> +            reg = <0 0x2b420000 0 0x1000>,
> +                  <0 0x2b430000 0 0x1000>;
> +            interrupts = <GIC_SPI 80 IRQ_TYPE_EDGE_RISING>;
> +            msi-parent = <&its 0xff0000>;
> +    };
> +
> +    tbu0: smmu-pmcg@2b440000 {
> +            compatible = "arm,smmu-v3-pmcg";
> +            reg = <0 0x2b440000 0 0x1000>,
> +                  <0 0x2b450000 0 0x1000>;
> +            interrupts = <GIC_SPI 81 IRQ_TYPE_EDGE_RISING>;
> +            msi-parent = <&its 0xff0000>;
> +    };
> -- 
> 2.27.0
> 

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


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