[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <cc2eb772-ae0f-4861-a36e-304a5f45b1bf@quicinc.com>
Date: Fri, 1 Dec 2023 01:24:48 +0200
From: Georgi Djakov <quic_c_gdjako@...cinc.com>
To: Rob Herring <robh@...nel.org>
CC: <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<will@...nel.org>, <robin.murphy@....com>, <joro@...tes.org>,
<devicetree@...r.kernel.org>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <linux-arm-kernel@...ts.infradead.org>,
<iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <quic_cgoldswo@...cinc.com>,
<quic_sukadev@...cinc.com>, <quic_pdaly@...cinc.com>,
<quic_sudaraja@...cinc.com>, <djakov@...nel.org>
Subject: Re: [PATCH v2 1/6] dt-bindings: iommu: Add Translation Buffer Unit
bindings
Hi Rob,
Thanks for the feedback!
On 11/27/2023 8:13 PM, Rob Herring wrote:
> On Fri, Nov 17, 2023 at 08:27:25PM -0800, Georgi Djakov wrote:
>> The "apps_smmu" on the Qualcomm sdm845 platform is an implementation
>> of the SMMU-500, that consists of a single TCU (Translation Control
>> Unit) and multiple TBUs (Translation Buffer Units). The TCU is already
>> being described in the generic SMMU DT schema. Add also bindings for
>> the TBUs to describe their properties and resources that needs to be
>> managed in order to operate them.
>>
>> In this DT schema, the TBUs are modelled as child devices of the TCU
>> and each of them is described with it's register space, clocks, power
>> domains, interconnects etc.
>>
>> Signed-off-by: Georgi Djakov <quic_c_gdjako@...cinc.com>
>> ---
>> .../devicetree/bindings/iommu/arm,smmu.yaml | 25 ++++++
>> .../bindings/iommu/qcom,qsmmuv500-tbu.yaml | 89 +++++++++++++++++++
>> 2 files changed, 114 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>> index aa9e1c0895a5..f7f89be5f7a3 100644
>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>> @@ -231,6 +231,18 @@ properties:
>> enabled for any given device.
>> $ref: /schemas/types.yaml#/definitions/phandle
>>
>> + '#address-cells':
>> + enum: [ 1, 2 ]
>> +
>> + '#size-cells':
>> + enum: [ 1, 2 ]
>> +
>> + ranges: true
>> +
>> +patternProperties:
>> + "^tbu@[0-9a-f]*":
>> + type: object
>> +
>> required:
>> - compatible
>> - reg
>> @@ -453,6 +465,19 @@ allOf:
>> - description: Voter clock required for HLOS SMMU access
>> - description: Interface clock required for register access
>>
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + const: qcom,smmu-500
>
> Doesn't match your example. No failure either, so there's some problem
> with your schema. The issue is the tbu@ entry above has no constraint on
> child properties. Dropping it would solve the issue. However, having a
> TBU is not QCom specific, so that doesn't feel right.
Having a TBU is not Qcom specific. The ARM MMU-500 implementation for example has TBUs, but the registers are within the SMMU address space, there are no clocks, no power-domains or other resources. Not sure about other implementations. So should we just allow empty tbu child nodes with no properties?
>> + then:
>> + patternProperties:
>> + "^tbu@[0-9a-f]*":
>
> '+' rather than '*' as 1 is the minimum, not 0.
Ok.
>> + $ref: qcom,qsmmuv500-tbu.yaml
>> + unevaluatedProperties: false
>> + properties:
>> + ranges: true
>> +
>> # Disallow clocks for all other platforms with specific compatibles
>> - if:
>> properties:
>> diff --git a/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml b/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
>> new file mode 100644
>> index 000000000000..4dc9d0ca33c9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iommu/qcom,qsmmuv500-tbu.yaml
>> @@ -0,0 +1,89 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iommu/qcom,qsmmuv500-tbu.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm TBU (Translation Buffer Unit)
>> +
>> +maintainers:
>> + - Georgi Djakov <quic_c_gdjako@...cinc.com>
>> +
>> +description:
>> + The Qualcomm SMMU500 implementation consists of TCU and TBU. The TBU contains
>> + a Translation Lookaside Buffer (TLB) that caches page tables. TBUs provides
>> + debug features to trace and trigger debug transactions. There are multiple TBU
>> + instances distributes with each client core.
>> +
>> +properties:
>> + $nodename:
>> + pattern: "^tbu@[0-9a-f]+$"
>
> Drop. You defined this in the parent already.
Ok.
>> +
>> + compatible:
>> + const: qcom,qsmmuv500-tbu
>> +
>> + reg:
>> + items:
>> + - description: Address and size of the TBU's register space.
>> +
>> + reg-names:
>> + items:
>> + - const: base
>
> Not a useful name. Drop.
Agree.
>> +
>> + clocks:
>> + maxItems: 1
>> +
>> + interconnects:
>> + maxItems: 1
>> +
>> + power-domains:
>> + maxItems: 1
>> +
>> + qcom,stream-id-range:
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + description: Stream ID range (address and size) that is assigned by the TBU
>> + items:
>> + minItems: 2
>> + maxItems: 2
>
> Perhaps implementations other than QCom's needs this?
Yes, maybe. A TBU can service a fixed amount of stream IDs and this looks like something common for all TBUs. I'll drop the vendor prefix.
Thanks,
Georgi
Powered by blists - more mailing lists