| Message ID | 20211116113536.69758-2-jean-philippe@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | perf/smmuv3: Support devicetree | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On Tue, 16 Nov 2021 11:35:36 +0000, Jean-Philippe Brucker wrote: > Add binding for the Arm SMMUv3 PMU. Each node represents a PMCG, and is > placed as a sibling node of the SMMU. Although the PMCGs registers may > be within the SMMU MMIO region, they are separate devices, and there can > be multiple PMCG devices for each SMMU (for example one for the TCU and > one for each TBU). > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > .../bindings/iommu/arm,smmu-v3-pmcg.yaml | 67 +++++++++++++++++++ > 1 file changed, 67 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.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/iommu/arm,smmu-v3-pmcg.yaml:24:9: [warning] wrong indentation: expected 10 but found 8 (indentation) ./Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml:25:11: [warning] wrong indentation: expected 12 but found 10 (indentation) dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.example.dt.yaml: example-0: pmu@2b420000:reg:0: [0, 725745664, 0, 4096] is too long From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.example.dt.yaml: example-0: pmu@2b420000:reg:1: [0, 725811200, 0, 4096] is too long From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.example.dt.yaml: example-0: pmu@2b440000:reg:0: [0, 725876736, 0, 4096] is too long From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.example.dt.yaml: example-0: pmu@2b440000:reg:1: [0, 725942272, 0, 4096] is too long From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1555758 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. 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.
On Tue, Nov 16, 2021 at 08:02:53AM -0600, Rob Herring wrote: > 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/iommu/arm,smmu-v3-pmcg.yaml:24:9: [warning] wrong indentation: expected 10 but found 8 (indentation) > ./Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml:25:11: [warning] wrong indentation: expected 12 but found 10 (indentation) > > dtschema/dtc warnings/errors: > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.example.dt.yaml: example-0: pmu@2b420000:reg:0: [0, 725745664, 0, 4096] is too long > From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.example.dt.yaml: example-0: pmu@2b420000:reg:1: [0, 725811200, 0, 4096] is too long > From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.example.dt.yaml: example-0: pmu@2b440000:reg:0: [0, 725876736, 0, 4096] is too long > From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.example.dt.yaml: example-0: pmu@2b440000:reg:1: [0, 725942272, 0, 4096] is too long > From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml > > doc reference errors (make refcheckdocs): > > See https://patchwork.ozlabs.org/patch/1555758 > > This check can fail if there are any dependencies. The base for a patch > series is generally the most recent rc1. > > 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. Right I'll fix those, I had only run dtbs_check Thanks, Jean
On Tue, Nov 16, 2021 at 5:52 AM Jean-Philippe Brucker <jean-philippe@linaro.org> wrote: > > Add binding for the Arm SMMUv3 PMU. Each node represents a PMCG, and is > placed as a sibling node of the SMMU. Although the PMCGs registers may > be within the SMMU MMIO region, they are separate devices, and there can > be multiple PMCG devices for each SMMU (for example one for the TCU and > one for each TBU). > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > .../bindings/iommu/arm,smmu-v3-pmcg.yaml | 67 +++++++++++++++++++ > 1 file changed, 67 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..a893e071fdb4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml > @@ -0,0 +1,67 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%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: |+ Don't need '|+' if no formatting to preserve. > + 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. Humm, I don't know that I agree they are standalone. They could be I guess, but looking at the MMU-600 spec the PMCG looks like it's just a subset of registers in a larger block. This seems similar to MPAM (which I'm working on a binding for) where it's just a register map and interrupts, but every other possible resource is unspecified by the architecture. The simplest change from this would be just specifying that the PMCG is child node(s) of whatever it is part of. The extreme would be this is all part of the SMMU binding (i.e. reg entry X is PMCG registers, interrupts entry Y is pmu irq). > + > +properties: > + $nodename: > + pattern: "^pmu@[0-9a-f]*" s/*/+/ Need at least 1 digit. > + compatible: > + oneOf: > + - items: > + - enum: > + - hisilicon,smmu-v3-pmcg-hip08 > + - const: arm,smmu-v3-pmcg > + - const: arm,smmu-v3-pmcg > + > + reg: > + description: | > + Base addresses of the PMCG registers. Either a single address for Page 0 > + or an additional address for Page 1, where some registers can be > + relocated with SMMU_PMCG_CFGR.RELOC_CTRS. > + minItems: 1 > + maxItems: 2 > + > + 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> > + > + pmu@2b420000 { > + 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>; > + }; > + > + pmu@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.33.1 >
On 2021-11-17 23:19, Rob Herring wrote: > On Tue, Nov 16, 2021 at 5:52 AM Jean-Philippe Brucker > <jean-philippe@linaro.org> wrote: >> >> Add binding for the Arm SMMUv3 PMU. Each node represents a PMCG, and is >> placed as a sibling node of the SMMU. Although the PMCGs registers may >> be within the SMMU MMIO region, they are separate devices, and there can >> be multiple PMCG devices for each SMMU (for example one for the TCU and >> one for each TBU). >> >> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> >> --- >> .../bindings/iommu/arm,smmu-v3-pmcg.yaml | 67 +++++++++++++++++++ >> 1 file changed, 67 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..a893e071fdb4 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml >> @@ -0,0 +1,67 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%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: |+ > > Don't need '|+' if no formatting to preserve. > >> + 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. > > Humm, I don't know that I agree they are standalone. They could be I > guess, but looking at the MMU-600 spec the PMCG looks like it's just a > subset of registers in a larger block. This seems similar to MPAM > (which I'm working on a binding for) where it's just a register map > and interrupts, but every other possible resource is unspecified by > the architecture. They're "standalone" in the sense that they don't have to be part of an SMMU, they could be part of a PCIe root complex or other SoC device that couples to an SMMU (e.g. anything that can speak AMBA DTI, in the case of our SMMU implementations). In fact our SMMU TBUs are pretty much separate devices themselves, they just *only* speak DTI, so access to their registers is proxied through the TCU programming interface. > The simplest change from this would be just specifying that the PMCG > is child node(s) of whatever it is part of. The extreme would be this > is all part of the SMMU binding (i.e. reg entry X is PMCG registers, > interrupts entry Y is pmu irq). Being a child of its associated device doesn't seem too bad semantically, however how would we describe a PMCG as a child of a PCIe node when its "reg" property still exists in the parent address space and not PCI config/memory space like any of its siblings? Also in practical terms, consuming that binding in Linux and getting the things to probe when it may want to be independent of whether we even understand the parent node at all could be... unpleasant. Robin. >> + >> +properties: >> + $nodename: >> + pattern: "^pmu@[0-9a-f]*" > > s/*/+/ > > Need at least 1 digit. > >> + compatible: >> + oneOf: >> + - items: >> + - enum: >> + - hisilicon,smmu-v3-pmcg-hip08 >> + - const: arm,smmu-v3-pmcg >> + - const: arm,smmu-v3-pmcg >> + >> + reg: >> + description: | >> + Base addresses of the PMCG registers. Either a single address for Page 0 >> + or an additional address for Page 1, where some registers can be >> + relocated with SMMU_PMCG_CFGR.RELOC_CTRS. >> + minItems: 1 >> + maxItems: 2 >> + >> + 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> >> + >> + pmu@2b420000 { >> + 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>; >> + }; >> + >> + pmu@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.33.1 >>
On Thu, Nov 18, 2021 at 03:50:54PM +0000, Robin Murphy wrote: > > > + 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. > > > > Humm, I don't know that I agree they are standalone. They could be I > > guess, but looking at the MMU-600 spec the PMCG looks like it's just a > > subset of registers in a larger block. This seems similar to MPAM > > (which I'm working on a binding for) where it's just a register map > > and interrupts, but every other possible resource is unspecified by > > the architecture. > > They're "standalone" in the sense that they don't have to be part of an > SMMU, they could be part of a PCIe root complex or other SoC device that > couples to an SMMU (e.g. anything that can speak AMBA DTI, in the case of > our SMMU implementations). The "standalone" word came from the SMMUv3 spec (IHI0070D.b 10.1): The Performance Monitor Counter Groups are standalone monitoring facilities and, as such, can be implemented in separate components that are all associated with (but not necessarily part of) an SMMU. > > In fact our SMMU TBUs are pretty much separate devices themselves, they just > *only* speak DTI, so access to their registers is proxied through the TCU > programming interface. > > > The simplest change from this would be just specifying that the PMCG > > is child node(s) of whatever it is part of. The extreme would be this > > is all part of the SMMU binding (i.e. reg entry X is PMCG registers, > > interrupts entry Y is pmu irq). > > Being a child of its associated device doesn't seem too bad semantically, > however how would we describe a PMCG as a child of a PCIe node when its > "reg" property still exists in the parent address space and not PCI > config/memory space like any of its siblings? Also in practical terms, > consuming that binding in Linux and getting the things to probe when it may > want to be independent of whether we even understand the parent node at all > could be... unpleasant. So there are multiple options for what "the PMCG is part of". (a) The SMMU: the spec guarantees that a PMCG is associated with an SMMU. (b) The MMIO region: may be within the SMMU (as with MMU-600), outside of it (as does another implementation, two 64k pages after the SMMU base) or, theoretically, within a separate device (e.g. PCIe controller). (c) The thing being measured: does not necessarily match the MMIO region. For example a TBU attached to the PCIe RC but the PMCG MMIO is within the SMMU region. (d) None: the PMCG can be probed and driven separately from the SMMU and other components, as demonstrated by Linux. Which one is normally picked to decide where to insert a devicetree node? I guess (b)? I picked (d) so far as the easiest choice. (a) is also a reasonable choice, being based on the spec, but it might be confusing to have a PMCG node inside the SMMU node when the MMIO region is external, possibly belonging to another device. For the same reason we could discard (c). (b) feels more natural, although it's not clear what to do when the PMCG MMIO region is external or adjacent to the SMMU region. Does the node go inside the SMMU node or one level up? Thanks, Jean > > Robin. > > > > + > > > +properties: > > > + $nodename: > > > + pattern: "^pmu@[0-9a-f]*" > > > > s/*/+/ > > > > Need at least 1 digit. > > > > > + compatible: > > > + oneOf: > > > + - items: > > > + - enum: > > > + - hisilicon,smmu-v3-pmcg-hip08 > > > + - const: arm,smmu-v3-pmcg > > > + - const: arm,smmu-v3-pmcg > > > + > > > + reg: > > > + description: | > > > + Base addresses of the PMCG registers. Either a single address for Page 0 > > > + or an additional address for Page 1, where some registers can be > > > + relocated with SMMU_PMCG_CFGR.RELOC_CTRS. > > > + minItems: 1 > > > + maxItems: 2 > > > + > > > + 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> > > > + > > > + pmu@2b420000 { > > > + 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>; > > > + }; > > > + > > > + pmu@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.33.1 > > >
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..a893e071fdb4 --- /dev/null +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml @@ -0,0 +1,67 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%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: "^pmu@[0-9a-f]*" + compatible: + oneOf: + - items: + - enum: + - hisilicon,smmu-v3-pmcg-hip08 + - const: arm,smmu-v3-pmcg + - const: arm,smmu-v3-pmcg + + reg: + description: | + Base addresses of the PMCG registers. Either a single address for Page 0 + or an additional address for Page 1, where some registers can be + relocated with SMMU_PMCG_CFGR.RELOC_CTRS. + minItems: 1 + maxItems: 2 + + 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> + + pmu@2b420000 { + 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>; + }; + + pmu@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>; + };
Add binding for the Arm SMMUv3 PMU. Each node represents a PMCG, and is placed as a sibling node of the SMMU. Although the PMCGs registers may be within the SMMU MMIO region, they are separate devices, and there can be multiple PMCG devices for each SMMU (for example one for the TCU and one for each TBU). Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> --- .../bindings/iommu/arm,smmu-v3-pmcg.yaml | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml