diff mbox series

[v5,1/7] dt-bindings: iommu: Add Qualcomm TBU bindings

Message ID 20240226172218.69486-2-quic_c_gdjako@quicinc.com
State Superseded
Headers show
Series Add support for Translation Buffer Units | expand

Commit Message

Georgi Djakov Feb. 26, 2024, 5:22 p.m. UTC
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). These TBUs have
hardware debugging features that are specific and only present on
Qualcomm hardware. Represent them as independent DT nodes. List all
the resources that are needed to operate them (such as registers,
clocks, power domains and interconnects).

Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
---
 .../devicetree/bindings/iommu/qcom,tbu.yaml   | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/qcom,tbu.yaml

Comments

Krzysztof Kozlowski Feb. 29, 2024, 5:53 p.m. UTC | #1
On 26/02/2024 18:22, 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). These TBUs have
> hardware debugging features that are specific and only present on
> Qualcomm hardware. Represent them as independent DT nodes. List all
> the resources that are needed to operate them (such as registers,
> clocks, power domains and interconnects).
> 
> Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
> ---
>  .../devicetree/bindings/iommu/qcom,tbu.yaml   | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/qcom,tbu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iommu/qcom,tbu.yaml b/Documentation/devicetree/bindings/iommu/qcom,tbu.yaml
> new file mode 100644
> index 000000000000..6841ca9af21f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/qcom,tbu.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iommu/qcom,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 with each client core.
> +
> +properties:
> +  compatible:
> +    const: qcom,qsmmuv500-tbu

Why we don't have SoC specific compatibles? If that's for SDM845, then
it should be qcom,sdm845-tbu or qcom,sdm845-qsmmuv500-tbu


> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  interconnects:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  qcom,stream-id-range:
> +    description: Phandle of a SMMU device and Stream ID range (address and size) that is assigned by the TBU

Please wrap it according to coding style, so 80.

> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle of a smmu node
> +          - description: stream id base address
> +          - description: stream id size
> +
> +required:
> +  - compatible
> +  - reg
> +  - qcom,stream-id-range
> +
> +unevaluatedProperties: false

This should be additionalProperties: false

> +
> +examples:
Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 29, 2024, 5:59 p.m. UTC | #2
On 26/02/2024 18:22, 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). These TBUs have
> hardware debugging features that are specific and only present on
> Qualcomm hardware. Represent them as independent DT nodes. List all
> the resources that are needed to operate them (such as registers,
> clocks, power domains and interconnects).
> 
> Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
> ---
>  .../devicetree/bindings/iommu/qcom,tbu.yaml   | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/qcom,tbu.yaml

Heh, I wonder how did you solve Robin's comments. I don't see you
responding to Robin. Just v5 sent...


Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 29, 2024, 6:01 p.m. UTC | #3
On 26/02/2024 18:22, 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). These TBUs have
> hardware debugging features that are specific and only present on
> Qualcomm hardware. Represent them as independent DT nodes. List all
> the resources that are needed to operate them (such as registers,
> clocks, power domains and interconnects).
> 
> Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>

Also one more nit, since I expect new version:

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

Best regards,
Krzysztof
Georgi Djakov Feb. 29, 2024, 8:09 p.m. UTC | #4
Hi Krzysztof,

On 29.02.24 19:53, Krzysztof Kozlowski wrote:
> On 26/02/2024 18:22, 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). These TBUs have
>> hardware debugging features that are specific and only present on
>> Qualcomm hardware. Represent them as independent DT nodes. List all
>> the resources that are needed to operate them (such as registers,
>> clocks, power domains and interconnects).
>>
>> Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
>> ---
>>   .../devicetree/bindings/iommu/qcom,tbu.yaml   | 65 +++++++++++++++++++
>>   1 file changed, 65 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iommu/qcom,tbu.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/qcom,tbu.yaml b/Documentation/devicetree/bindings/iommu/qcom,tbu.yaml
>> new file mode 100644
>> index 000000000000..6841ca9af21f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iommu/qcom,tbu.yaml
>> @@ -0,0 +1,65 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iommu/qcom,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 with each client core.
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,qsmmuv500-tbu
> 
> Why we don't have SoC specific compatibles? If that's for SDM845, then
> it should be qcom,sdm845-tbu or qcom,sdm845-qsmmuv500-tbu
> 

Because they should be all compatible (as registers). Adding a SoC compatible
might get overly-specific, but i can also see the benefits in that, so ok will
do it!

> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  interconnects:
>> +    maxItems: 1
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  qcom,stream-id-range:
>> +    description: Phandle of a SMMU device and Stream ID range (address and size) that is assigned by the TBU
> 
> Please wrap it according to coding style, so 80.
> 

Sure, thanks!

>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    items:
>> +      - items:
>> +          - description: phandle of a smmu node
>> +          - description: stream id base address
>> +          - description: stream id size
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - qcom,stream-id-range
>> +
>> +unevaluatedProperties: false
> 
> This should be additionalProperties: false
> 

Ok right, thanks for taking a look!

BR,
Georgi
Georgi Djakov Feb. 29, 2024, 8:17 p.m. UTC | #5
Hi Krzysztof,

On 29.02.24 19:59, Krzysztof Kozlowski wrote:
> On 26/02/2024 18:22, 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). These TBUs have
>> hardware debugging features that are specific and only present on
>> Qualcomm hardware. Represent them as independent DT nodes. List all
>> the resources that are needed to operate them (such as registers,
>> clocks, power domains and interconnects).
>>
>> Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
>> ---
>>   .../devicetree/bindings/iommu/qcom,tbu.yaml   | 65 +++++++++++++++++++
>>   1 file changed, 65 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iommu/qcom,tbu.yaml
> 
> Heh, I wonder how did you solve Robin's comments. I don't see you
> responding to Robin. Just v5 sent...

Yeah, i didn't respond because his response was clear to me. He responded
to the fundamental question whether to model the TBUs as child DT nodes of
the SMMU or as standalone nodes. So in the first versions of this patchset
we tried to explore the path with "child" nodes and search if there are any
other implementation than the Qualcomm one and try to find some common
binding... and Robin's objection was exactly to that. It seems that adding
more functionalities in TBUs (which requires resource management) is only
a Qualcomm thing and common binding does not make sense, so now we are
going with standalone DT nodes as he suggested.

Thanks,
Georgi
Chris Goldsworthy Feb. 29, 2024, 10:24 p.m. UTC | #6
On Thu, Feb 29, 2024 at 10:09:34PM +0200, Georgi Djakov wrote:
> Hi Krzysztof,
> 
> On 29.02.24 19:53, Krzysztof Kozlowski wrote:
> >On 26/02/2024 18:22, Georgi Djakov wrote:
> >>+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>+%YAML 1.2
> >>+---
> >>+$id: http://devicetree.org/schemas/iommu/qcom,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 with each client core.
> >>+
> >>+properties:
> >>+  compatible:
> >>+    const: qcom,qsmmuv500-tbu
> >
> >Why we don't have SoC specific compatibles? If that's for SDM845, then
> >it should be qcom,sdm845-tbu or qcom,sdm845-qsmmuv500-tbu
> >
> 
> Because they should be all compatible (as registers). Adding a SoC compatible
> might get overly-specific, but i can also see the benefits in that, so ok will
> do it!
> 

Hi Krzysztof,

JFYI that the TBUs are used on our mobile SoCs going up until the SoC
we commercialized in early 2022, Snapdragon 8 Gen 1. Including SDM845
there are three more premium tier SoCs using TBUs plus all of their
value-tier derivatives.  There will also be prior generation premium
tier SoCs along with their derivatives that use the TBU as well. Does
it make sense to have a target-specific compatible string given this? 

Thanks,

Chris.
Krzysztof Kozlowski March 1, 2024, 6:32 a.m. UTC | #7
On 29/02/2024 23:24, Chris Goldsworthy wrote:
> On Thu, Feb 29, 2024 at 10:09:34PM +0200, Georgi Djakov wrote:
>> Hi Krzysztof,
>>
>> On 29.02.24 19:53, Krzysztof Kozlowski wrote:
>>> On 26/02/2024 18:22, Georgi Djakov wrote:
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/iommu/qcom,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 with each client core.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: qcom,qsmmuv500-tbu
>>>
>>> Why we don't have SoC specific compatibles? If that's for SDM845, then
>>> it should be qcom,sdm845-tbu or qcom,sdm845-qsmmuv500-tbu
>>>
>>
>> Because they should be all compatible (as registers). Adding a SoC compatible
>> might get overly-specific, but i can also see the benefits in that, so ok will
>> do it!
>>
> 
> Hi Krzysztof,
> 
> JFYI that the TBUs are used on our mobile SoCs going up until the SoC
> we commercialized in early 2022, Snapdragon 8 Gen 1. Including SDM845
> there are three more premium tier SoCs using TBUs plus all of their
> value-tier derivatives.  There will also be prior generation premium
> tier SoCs along with their derivatives that use the TBU as well. Does
> it make sense to have a target-specific compatible string given this? 

This does not explain me anything. Why an exemption from usual bindings
rules should apply here?

https://elixir.bootlin.com/linux/v6.8-rc6/source/Documentation/devicetree/bindings/writing-bindings.rst

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iommu/qcom,tbu.yaml b/Documentation/devicetree/bindings/iommu/qcom,tbu.yaml
new file mode 100644
index 000000000000..6841ca9af21f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/qcom,tbu.yaml
@@ -0,0 +1,65 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iommu/qcom,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 with each client core.
+
+properties:
+  compatible:
+    const: qcom,qsmmuv500-tbu
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  interconnects:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  qcom,stream-id-range:
+    description: Phandle of a SMMU device and Stream ID range (address and size) that is assigned by the TBU
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: phandle of a smmu node
+          - description: stream id base address
+          - description: stream id size
+
+required:
+  - compatible
+  - reg
+  - qcom,stream-id-range
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+    #include <dt-bindings/interconnect/qcom,icc.h>
+    #include <dt-bindings/interconnect/qcom,sdm845.h>
+
+    tbu@150e1000 {
+        compatible = "qcom,qsmmuv500-tbu";
+        reg = <0x150e1000 0x1000>;
+        clocks = <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>;
+        interconnects = <&system_noc MASTER_GNOC_SNOC QCOM_ICC_TAG_ACTIVE_ONLY
+                         &config_noc SLAVE_IMEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>;
+        power-domains = <&gcc HLOS1_VOTE_AGGRE_NOC_MMU_PCIE_TBU_GDSC>;
+        qcom,stream-id-range = <&apps_smmu 0x1c00 0x400>;
+    };
+...