Re: [PATCH v2 1/3] dt-bindings: display: tegra: document EPP, ISP, MPE and TSEC for Tegra114 and Tegra124

From: Krzysztof Kozlowski
Date: Fri Mar 07 2025 - 09:07:44 EST

On 07/03/2025 09:10, Svyatoslav Ryhel wrote:
> The current EPP, ISP and MPE schemas are largely compatible with Tegra114 and Tegra124,
> requiring only minor adjustments. Additionally, the TSEC schema for the Security engine,
> which is available from Tegra114 onwards, is included.


Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

>
> Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> ---
> .../display/tegra/nvidia,tegra114-tsec.yaml | 70 +++++++++++++++++++
> .../display/tegra/nvidia,tegra20-epp.yaml | 12 ++--
> .../display/tegra/nvidia,tegra20-isp.yaml | 16 +++--
> .../display/tegra/nvidia,tegra20-mpe.yaml | 30 ++++++--
> 4 files changed, 114 insertions(+), 14 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/display/tegra/nvidia,tegra114-tsec.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra114-tsec.yaml b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra114-tsec.yaml
> new file mode 100644
> index 000000000000..84d9ab9394d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra114-tsec.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/tegra/nvidia,tegra114-tsec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NVIDIA Tegra Security co-processor

How security processor is related to display?



> +
> +maintainers:
> + - Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> + - Thierry Reding <thierry.reding@xxxxxxxxx>

Please provide description of the hardware here.

> +
> +properties:
> + compatible:
> + oneOf:
> + - const: nvidia,tegra114-tsec
> + - const: nvidia,tegra124-tsec

That's just enum

> + - items:
> + - const: nvidia,tegra132-tsec
> + - const: nvidia,tegra124-tsec
> + - const: nvidia,tegra210-tsec

And this goes to enum.

> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: module clock

Instead: maxItems: 1

> +
> + resets:
> + items:
> + - description: module reset

Instead: maxItems: 1


> +
> + reset-names:
> + items:
> + - const: tsec

Drop reset-names, redundant.

> +
> + iommus:
> + maxItems: 1
> +
> + interconnects:
> + maxItems: 6

You need to list the items.

> +
> + interconnect-names:
> + maxItems: 6

You need to list the items.

> +
> + operating-points-v2: true

No opp-table?

> +
> + power-domains:
> + items:
> + - description: phandle to the core power domain
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/tegra114-car.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + tsec@54500000 {
> + compatible = "nvidia,tegra114-tsec";
> + reg = <0x54500000 0x00040000>;
> + interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&tegra_car TEGRA114_CLK_TSEC>;
> + resets = <&tegra_car TEGRA114_CLK_TSEC>;
> + reset-names = "tsec";
> + };
> diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-epp.yaml b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-epp.yaml
> index 3c095a5491fe..a50e3261a191 100644
> --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-epp.yaml
> +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-epp.yaml
> @@ -15,10 +15,14 @@ properties:
> pattern: "^epp@[0-9a-f]+$"
>
> compatible:
> - enum:
> - - nvidia,tegra20-epp
> - - nvidia,tegra30-epp
> - - nvidia,tegra114-epp
> + oneOf:
> + - const: nvidia,tegra20-epp
> + - const: nvidia,tegra30-epp
> + - const: nvidia,tegra114-epp
> + - const: nvidia,tegra124-epp

No, that was an enum.

> + - items:
> + - const: nvidia,tegra132-epp
> + - const: nvidia,tegra124-epp
>
> reg:
> maxItems: 1
> diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-isp.yaml b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-isp.yaml
> index 3bc3b22e98e1..bfef4f26a3d7 100644
> --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-isp.yaml
> +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-isp.yaml
> @@ -11,11 +11,19 @@ maintainers:
> - Jon Hunter <jonathanh@xxxxxxxxxx>
>
> properties:
> + $nodename:
> + pattern: "^isp@[0-9a-f]+$"

Why?

Nothing in commit msg explains this.


> +
> compatible:
> - enum:
> - - nvidia,tegra20-isp
> - - nvidia,tegra30-isp
> - - nvidia,tegra210-isp
> + oneOf:
> + - const: nvidia,tegra20-isp
> + - const: nvidia,tegra30-isp
> + - const: nvidia,tegra114-isp
> + - const: nvidia,tegra124-isp
> + - items:
> + - const: nvidia,tegra132-isp
> + - const: nvidia,tegra124-isp
> + - const: nvidia,tegra210-isp
>
> reg:
> maxItems: 1
> diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-mpe.yaml b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-mpe.yaml
> index 2cd3e60cd0a8..35e3991f1135 100644
> --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-mpe.yaml
> +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-mpe.yaml
> @@ -12,13 +12,19 @@ maintainers:
>
> properties:
> $nodename:
> - pattern: "^mpe@[0-9a-f]+$"
> + oneOf:
> + - pattern: "^mpe@[0-9a-f]+$"
> + - pattern: "^msenc@[0-9a-f]+$"
>
> compatible:
> - enum:
> - - nvidia,tegra20-mpe
> - - nvidia,tegra30-mpe
> - - nvidia,tegra114-mpe
> + oneOf:
> + - const: nvidia,tegra20-mpe
> + - const: nvidia,tegra30-mpe
> + - const: nvidia,tegra114-msenc
> + - const: nvidia,tegra124-msenc
> + - items:
> + - const: nvidia,tegra132-msenc
> + - const: nvidia,tegra124-msenc
>
> reg:
> maxItems: 1
> @@ -36,7 +42,9 @@ properties:
>
> reset-names:
> items:
> - - const: mpe
> + - enum:
> + - mpe
> + - msenc

No, why? That's redundant. Having names equal to module name brings zero
benefits. This change even shows that it is counter productive.

>
> iommus:
> maxItems: 1
> @@ -58,6 +66,7 @@ additionalProperties: false
> examples:
> - |
> #include <dt-bindings/clock/tegra20-car.h>
> + #include <dt-bindings/clock/tegra114-car.h>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
>
> mpe@54040000 {
> @@ -68,3 +77,12 @@ examples:
> resets = <&tegra_car 60>;
> reset-names = "mpe";
> };
> +
> + msenc@544c0000 {
> + compatible = "nvidia,tegra114-msenc";

Difference in compatible does not mean you need new example.


Best regards,
Krzysztof