| Message ID | c14404f91371dd2b2f194ea92cb092602bcb393e.1645106346.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: Arm SMMU PMU driver | expand |
Send DT patches to the DT list if you want them reviewed with some guarantee that I'll see them. (Except right now as PW stopped getting mail. :( ) On Thu, Feb 17, 2022 at 02:24:17PM +0000, Robin Murphy wrote: > Since IORT describes PMU interrupts rather inflexibly as an inherent What's IORT? ;) > part of the SMMU, the best way to avoid excessive complexity is to make > the way we handle DT look as similar as possible. Fortunately the > de-facto standard for mentioning PMU interrupts at all under the current > binding has been to include them in the global interrupt count, listing > them after the actual fault interrupt(s), so we can capitalise on that. > It's about 9 years too late to redefine "#global-interrupts" to exclude > anything other than context interrupts without breaking compatibility, > so we're stuck with a slightly convoluted definition of PMU interrupts > as an optional subdivision of the "global" interrupts, but it works. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > --- > > Not sure whether the count-backwards-from-the-middle nature of "number > of PMU interrupts" is too ugly and "index of first PMU interrupt" might > be any better. Can this be solved with 'interrupt-names' instead deprecating #global-interrupts along the way? > --- > .../devicetree/bindings/iommu/arm,smmu.yaml | 19 ++++++++++++++++++- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 4 ++++ > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > index da5381c8ee11..9d39df42528a 100644 > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > @@ -87,10 +87,18 @@ properties: > > '#global-interrupts': > description: The number of global interrupts exposed by the device. > + Includes the count of PMU interrupts, if present. > $ref: /schemas/types.yaml#/definitions/uint32 > minimum: 0 > maximum: 260 # 2 secure, 2 non-secure, and up to 256 perf counters > > + '#pmu-interrupts': It would have been great if everything that's a count/size used '#', but that didn't happen. So I'm not that wild about using '#' randomly at all. This needs a vendor prefix (arm,). > + description: The number of PMU interrupts. Must be equal to the number of > + implemented counter groups. > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 1 > + maximum: 256 > + > '#iommu-cells': > enum: [ 1, 2 ] > description: | > @@ -115,6 +123,10 @@ properties: > context bank. In the case of a single, combined interrupt, it must be > listed multiple times. > > + If a PMU is present, the global interrupt entries consist of any fault > + interrupts first, followed by #pmu-interrupts entries, one per implemented > + counter group, specified in order of their indexing by the SMMU. > + > dma-coherent: > description: | > Present if page table walks made by the SMMU are cache coherent with the > @@ -190,9 +202,14 @@ examples: > smmu1: iommu@ba5e0000 { > compatible = "arm,smmu-v1"; > reg = <0xba5e0000 0x10000>; > - #global-interrupts = <2>; > + #global-interrupts = <6>; /* 2 fault + 4 PMU */ > + #pmu-interrupts = <4>; > interrupts = <0 32 4>, > <0 33 4>, > + <0 94 4>, /* This is the first PMU interrupt */ > + <0 95 4>, > + <0 96 4>, > + <0 97 4>, > <0 34 4>, /* This is the first context interrupt */ > <0 35 4>, > <0 36 4>, > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c > index cbfe4cc914f0..8d6c8106fc1d 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > @@ -1995,6 +1995,10 @@ static int arm_smmu_device_dt_probe(struct arm_smmu_device *smmu, > return dev_err_probe(dev, -ENODEV, > "missing #global-interrupts property\n"); > *pmu_irqs = 0; > + of_property_read_u32(dev->of_node, "#pmu-interrupts", pmu_irqs); > + if (*pmu_irqs > *global_irqs) > + return dev_err_probe(dev, -EINVAL, "invalid #pmu_interrupts property"); > + *global_irqs -= *pmu_irqs; > > data = of_device_get_match_data(dev); > smmu->version = data->version; > -- > 2.28.0.dirty > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 2022-03-02 15:33, Rob Herring wrote: > Send DT patches to the DT list if you want them reviewed with some > guarantee that I'll see them. (Except right now as PW stopped getting > mail. :( ) Oops, sorry, I think I had some sort of mental block there - I should equally have cc'd the IOMMU list too. > On Thu, Feb 17, 2022 at 02:24:17PM +0000, Robin Murphy wrote: >> Since IORT describes PMU interrupts rather inflexibly as an inherent > > What's IORT? ;) See the preceding patch :P But indeed this particular sentence is still its untouched 5-year-old self... today I'd tend to write "ACPI IORT", is that better? >> part of the SMMU, the best way to avoid excessive complexity is to make >> the way we handle DT look as similar as possible. Fortunately the >> de-facto standard for mentioning PMU interrupts at all under the current >> binding has been to include them in the global interrupt count, listing >> them after the actual fault interrupt(s), so we can capitalise on that. >> It's about 9 years too late to redefine "#global-interrupts" to exclude >> anything other than context interrupts without breaking compatibility, >> so we're stuck with a slightly convoluted definition of PMU interrupts >> as an optional subdivision of the "global" interrupts, but it works. >> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> >> --- >> >> Not sure whether the count-backwards-from-the-middle nature of "number >> of PMU interrupts" is too ugly and "index of first PMU interrupt" might >> be any better. > > Can this be solved with 'interrupt-names' instead deprecating > #global-interrupts along the way? My gut feeling is that that's not realistically feasible - we don't have a fixed set of IRQ lines with well-defined names, we have an imp-def set of "things that might raise one or more of global fault conditions", plus some number of context interrupts that work one of two ways, plus possibly some number of PMU counter group interrupts, with the additional fun that for some implementations those may all be the same physical signal (and I've now seen at least one SoC inexplicably wire up a combined interrupt *as well* as the individual outputs, and describe them all!) My main concern is not breaking backward-compatibility for a new DT with an old OS. From past experience, I'm pretty sure Xen would be up in arms about any breakage in that direction, so "#global-interrupts" is sadly going nowhere for the foreseeable future. However even just thinking about implementing such a thing in the Linux driver puts me off - we'd have to take some horrible backwards approach where we look at all the names first to figure out which IRQs to get, and such an abuse of "interrupt-names" sounds worse than what it's trying to avoid. The other approach I tried was to have a "pmu" sub-node with its own "interrupts" property, but that turned out to cause problems if it was behind an "interrupt-map", for reasons which didn't entirely make sense. Plus the corresponding DT changes were uglier and more invasive - the nice thing about the method here is that PMU support becomes a simple one-line addition to many existing DTs. >> --- >> .../devicetree/bindings/iommu/arm,smmu.yaml | 19 ++++++++++++++++++- >> drivers/iommu/arm/arm-smmu/arm-smmu.c | 4 ++++ >> 2 files changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml >> index da5381c8ee11..9d39df42528a 100644 >> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml >> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml >> @@ -87,10 +87,18 @@ properties: >> >> '#global-interrupts': >> description: The number of global interrupts exposed by the device. >> + Includes the count of PMU interrupts, if present. >> $ref: /schemas/types.yaml#/definitions/uint32 >> minimum: 0 >> maximum: 260 # 2 secure, 2 non-secure, and up to 256 perf counters >> >> + '#pmu-interrupts': > > It would have been great if everything that's a count/size used '#', but > that didn't happen. So I'm not that wild about using '#' randomly at > all. > > This needs a vendor prefix (arm,). Sure - I went for consistency with what's already established, but since I'm similarly none too fond of it, swapping for something like "arm,pmu-irq-start" is fine by me. Thanks, Robin. >> + description: The number of PMU interrupts. Must be equal to the number of >> + implemented counter groups. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 1 >> + maximum: 256 >> + >> '#iommu-cells': >> enum: [ 1, 2 ] >> description: | >> @@ -115,6 +123,10 @@ properties: >> context bank. In the case of a single, combined interrupt, it must be >> listed multiple times. >> >> + If a PMU is present, the global interrupt entries consist of any fault >> + interrupts first, followed by #pmu-interrupts entries, one per implemented >> + counter group, specified in order of their indexing by the SMMU. >> + >> dma-coherent: >> description: | >> Present if page table walks made by the SMMU are cache coherent with the >> @@ -190,9 +202,14 @@ examples: >> smmu1: iommu@ba5e0000 { >> compatible = "arm,smmu-v1"; >> reg = <0xba5e0000 0x10000>; >> - #global-interrupts = <2>; >> + #global-interrupts = <6>; /* 2 fault + 4 PMU */ >> + #pmu-interrupts = <4>; >> interrupts = <0 32 4>, >> <0 33 4>, >> + <0 94 4>, /* This is the first PMU interrupt */ >> + <0 95 4>, >> + <0 96 4>, >> + <0 97 4>, >> <0 34 4>, /* This is the first context interrupt */ >> <0 35 4>, >> <0 36 4>, >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c >> index cbfe4cc914f0..8d6c8106fc1d 100644 >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c >> @@ -1995,6 +1995,10 @@ static int arm_smmu_device_dt_probe(struct arm_smmu_device *smmu, >> return dev_err_probe(dev, -ENODEV, >> "missing #global-interrupts property\n"); >> *pmu_irqs = 0; >> + of_property_read_u32(dev->of_node, "#pmu-interrupts", pmu_irqs); >> + if (*pmu_irqs > *global_irqs) >> + return dev_err_probe(dev, -EINVAL, "invalid #pmu_interrupts property"); >> + *global_irqs -= *pmu_irqs; >> >> data = of_device_get_match_data(dev); >> smmu->version = data->version; >> -- >> 2.28.0.dirty >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml index da5381c8ee11..9d39df42528a 100644 --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml @@ -87,10 +87,18 @@ properties: '#global-interrupts': description: The number of global interrupts exposed by the device. + Includes the count of PMU interrupts, if present. $ref: /schemas/types.yaml#/definitions/uint32 minimum: 0 maximum: 260 # 2 secure, 2 non-secure, and up to 256 perf counters + '#pmu-interrupts': + description: The number of PMU interrupts. Must be equal to the number of + implemented counter groups. + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 1 + maximum: 256 + '#iommu-cells': enum: [ 1, 2 ] description: | @@ -115,6 +123,10 @@ properties: context bank. In the case of a single, combined interrupt, it must be listed multiple times. + If a PMU is present, the global interrupt entries consist of any fault + interrupts first, followed by #pmu-interrupts entries, one per implemented + counter group, specified in order of their indexing by the SMMU. + dma-coherent: description: | Present if page table walks made by the SMMU are cache coherent with the @@ -190,9 +202,14 @@ examples: smmu1: iommu@ba5e0000 { compatible = "arm,smmu-v1"; reg = <0xba5e0000 0x10000>; - #global-interrupts = <2>; + #global-interrupts = <6>; /* 2 fault + 4 PMU */ + #pmu-interrupts = <4>; interrupts = <0 32 4>, <0 33 4>, + <0 94 4>, /* This is the first PMU interrupt */ + <0 95 4>, + <0 96 4>, + <0 97 4>, <0 34 4>, /* This is the first context interrupt */ <0 35 4>, <0 36 4>, diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c index cbfe4cc914f0..8d6c8106fc1d 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c @@ -1995,6 +1995,10 @@ static int arm_smmu_device_dt_probe(struct arm_smmu_device *smmu, return dev_err_probe(dev, -ENODEV, "missing #global-interrupts property\n"); *pmu_irqs = 0; + of_property_read_u32(dev->of_node, "#pmu-interrupts", pmu_irqs); + if (*pmu_irqs > *global_irqs) + return dev_err_probe(dev, -EINVAL, "invalid #pmu_interrupts property"); + *global_irqs -= *pmu_irqs; data = of_device_get_match_data(dev); smmu->version = data->version;
Since IORT describes PMU interrupts rather inflexibly as an inherent part of the SMMU, the best way to avoid excessive complexity is to make the way we handle DT look as similar as possible. Fortunately the de-facto standard for mentioning PMU interrupts at all under the current binding has been to include them in the global interrupt count, listing them after the actual fault interrupt(s), so we can capitalise on that. It's about 9 years too late to redefine "#global-interrupts" to exclude anything other than context interrupts without breaking compatibility, so we're stuck with a slightly convoluted definition of PMU interrupts as an optional subdivision of the "global" interrupts, but it works. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- Not sure whether the count-backwards-from-the-middle nature of "number of PMU interrupts" is too ugly and "index of first PMU interrupt" might be any better. --- .../devicetree/bindings/iommu/arm,smmu.yaml | 19 ++++++++++++++++++- drivers/iommu/arm/arm-smmu/arm-smmu.c | 4 ++++ 2 files changed, 22 insertions(+), 1 deletion(-)