| Message ID | 20200706112246.92220-3-jkchen@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf/smmuv3: dts get opt and simplify code | expand |
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> > --- > drivers/perf/arm_smmuv3_pmu.c | 42 ++++++++++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c > index 2d09f3e47d12..44e9f4197444 100644 > --- a/drivers/perf/arm_smmuv3_pmu.c > +++ b/drivers/perf/arm_smmuv3_pmu.c > @@ -115,6 +115,16 @@ struct smmu_pmu { > bool global_filter; > }; > > +struct smmu_pmu_prop { > + u32 opt; > + const char *prop; > +}; > + > +static struct smmu_pmu_prop smmu_pmu_options[] = { > + { SMMU_PMCG_EVCNTR_RDONLY, "hisilicon,smmu-pmcg-evcntr-rdonly"}, We know this kind of thing doesn't scale well - please define a compatible string for the specific PMU model. In fact my hope was that DT compatibles would map to the same model values we made up for IORT to pass, such that the *only* firmware-specific difference the driver needs to have is whether to retrieve the model from platdata or OF match data, then the actual quirks are applied commonly. > + { 0, NULL}, > +}; > + > #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu)) > > #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end) \ > @@ -708,6 +718,7 @@ static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu) > smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0); > } > > +#ifdef CONFIG_ACPI Why bother stubbing this out? It shouldn't have any build-time dependency on ACPI code, and saving a whole 48 bytes from the object size (for my local build setup) struggles to justify the source-code clutter. > static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) > { > u32 model; > @@ -723,6 +734,26 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) > > dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options); > } > +#else > +static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) > +{ > + > +} > +#endif > + > +static void smmu_pmu_get_dt_options(struct smmu_pmu *smmu_pmu) > +{ > + int i = 0; > + > + do { > + if (of_property_read_bool(smmu_pmu->dev->of_node, > + smmu_pmu_options[i].prop)) { > + smmu_pmu->options |= smmu_pmu_options[i].opt; > + dev_notice(smmu_pmu->dev, "option mask 0x%x\n", > + smmu_pmu->options); > + } > + } while (smmu_pmu_options[++i].opt); > +} > > static int smmu_pmu_probe(struct platform_device *pdev) > { > @@ -801,7 +832,10 @@ static int smmu_pmu_probe(struct platform_device *pdev) > return -EINVAL; > } > > - smmu_pmu_get_acpi_options(smmu_pmu); > + if (dev->of_node) > + smmu_pmu_get_dt_options(smmu_pmu); > + else > + smmu_pmu_get_acpi_options(smmu_pmu); > > /* Pick one CPU to be the preferred one to use */ > smmu_pmu->on_cpu = raw_smp_processor_id(); > @@ -855,9 +889,15 @@ static void smmu_pmu_shutdown(struct platform_device *pdev) > smmu_pmu_disable(&smmu_pmu->pmu); > } > > +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... > + { }, > +}; How about the thing for module autoloading too? Robin. > + > static struct platform_driver smmu_pmu_driver = { > .driver = { > .name = "arm-smmu-v3-pmcg", > + .of_match_table = smmu_pmu_of_match, > }, > .probe = smmu_pmu_probe, > .remove = smmu_pmu_remove, >
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. 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 +%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]*" + compatible: + const: arm,smmu-v3-pmcg + + reg: + 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> + + tcu: smmu-pmcg@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>; + }; + + 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>; + };
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
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@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? 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@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. 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@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 >
diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c index 2d09f3e47d12..44e9f4197444 100644 --- a/drivers/perf/arm_smmuv3_pmu.c +++ b/drivers/perf/arm_smmuv3_pmu.c @@ -115,6 +115,16 @@ struct smmu_pmu { bool global_filter; }; +struct smmu_pmu_prop { + u32 opt; + const char *prop; +}; + +static struct smmu_pmu_prop smmu_pmu_options[] = { + { SMMU_PMCG_EVCNTR_RDONLY, "hisilicon,smmu-pmcg-evcntr-rdonly"}, + { 0, NULL}, +}; + #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu)) #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end) \ @@ -708,6 +718,7 @@ static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu) smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0); } +#ifdef CONFIG_ACPI static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) { u32 model; @@ -723,6 +734,26 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options); } +#else +static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) +{ + +} +#endif + +static void smmu_pmu_get_dt_options(struct smmu_pmu *smmu_pmu) +{ + int i = 0; + + do { + if (of_property_read_bool(smmu_pmu->dev->of_node, + smmu_pmu_options[i].prop)) { + smmu_pmu->options |= smmu_pmu_options[i].opt; + dev_notice(smmu_pmu->dev, "option mask 0x%x\n", + smmu_pmu->options); + } + } while (smmu_pmu_options[++i].opt); +} static int smmu_pmu_probe(struct platform_device *pdev) { @@ -801,7 +832,10 @@ static int smmu_pmu_probe(struct platform_device *pdev) return -EINVAL; } - smmu_pmu_get_acpi_options(smmu_pmu); + if (dev->of_node) + smmu_pmu_get_dt_options(smmu_pmu); + else + smmu_pmu_get_acpi_options(smmu_pmu); /* Pick one CPU to be the preferred one to use */ smmu_pmu->on_cpu = raw_smp_processor_id(); @@ -855,9 +889,15 @@ static void smmu_pmu_shutdown(struct platform_device *pdev) smmu_pmu_disable(&smmu_pmu->pmu); } +static const struct of_device_id smmu_pmu_of_match[] = { + { .compatible = "arm-smmu-v3-pmcg", }, + { }, +}; + static struct platform_driver smmu_pmu_driver = { .driver = { .name = "arm-smmu-v3-pmcg", + .of_match_table = smmu_pmu_of_match, }, .probe = smmu_pmu_probe, .remove = smmu_pmu_remove,
For the smmuv3 pmu for support the dts to get the options Signed-off-by: Jay Chen <jkchen@linux.alibaba.com> --- drivers/perf/arm_smmuv3_pmu.c | 42 ++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)