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

From: Rob Herring <robh@kernel.org>
To: Robin Murphy <robin.murphy@arm.com>
Cc: will@kernel.org, mark.rutland@arm.com, lorenzo.pieralisi@arm.com,
	sudeep.holla@arm.com, linux-arm-kernel@lists.infradead.org,
	jean-philippe@linaro.org, leo.yan@linaro.org,
	noda.akio@socionext.com
Subject: Re: [PATCH 3/6] iommu/arm-smmu: Add DT PMU support
Date: Wed, 2 Mar 2022 09:33:14 -0600	[thread overview]
Message-ID: <Yh+OOs0BStF0y/VF@robh.at.kernel.org> (raw)
In-Reply-To: <c14404f91371dd2b2f194ea92cb092602bcb393e.1645106346.git.robin.murphy@arm.com>

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

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