Re: [PATCH 3/6] iommu/arm-smmu: Add DT PMU support

[Robin Murphy <robin.murphy@arm.com>]

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

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