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@xxxxxxxxxxx> >> --- >> .../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@xxxxxxxxxxx> >> + >> +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
