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

[Robin Murphy]

On 2020-07-14 00:25, Rob Herring wrote:
> 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 at 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 at 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?

Yes, PMCGs are their own thing with their own little programming 
interfaces and interrupts, there are typically multiple PMCG instances 
per SMMU, and they may even belong to "non-SMMU" components which 
participate in translation, like PCIe root complexes or devices with 
their own embedded TLBs.

>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe at 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 at kernel.org>
>> +  - Robin Murphy <Robin.Murphy at 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 at ...
> 
> (or whatever we've used for PMUs).
> 
>> +  compatible:
>> +    const: arm,smmu-v3-pmcg
> 
> This is correct, but doesn't match the driver.

To be fair, this binding wasn't originally written for this particular 
driver patch ;)

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

The second is "optional" in terms of the architecture (and thus the 
generic compatible), but fixed for any specific implementation - it's a 
choice of whether the counter registers are in the same page as the 
control registers or in a separate page, but that can be architecturally 
discovered from an ID register in the first page (see the handling of 
SMMU_PMCG_CFGR_RELOC_CTRS if you're interested).
Robin.

>> +
>> +  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 at 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 at 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 at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>