Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <cc2eb772-ae0f-4861-a36e-304a5f45b1bf@quicinc.com>
Date:   Fri, 1 Dec 2023 01:24:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/6] dt-bindings: iommu: Add Translation Buffer Unit
 bindings
Content-Language: en-US
To:     Rob Herring <robh@kernel.org>
CC:     <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <will@kernel.org>, <robin.murphy@arm.com>, <joro@8bytes.org>,
        <devicetree@vger.kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <linux-arm-kernel@lists.infradead.org>,
        <iommu@lists.linux.dev>, <linux-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <quic_cgoldswo@quicinc.com>,
        <quic_sukadev@quicinc.com>, <quic_pdaly@quicinc.com>,
        <quic_sudaraja@quicinc.com>, <djakov@kernel.org>
References: <20231118042730.2799-1-quic_c_gdjako@quicinc.com>
 <20231118042730.2799-2-quic_c_gdjako@quicinc.com>
 <20231127181347.GA1953740-robh@kernel.org>
From:   Georgi Djakov <quic_c_gdjako@quicinc.com>
In-Reply-To: <20231127181347.GA1953740-robh@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@quicinc.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@quicinc.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