Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d11b17d8-bc36-3a64-3de0-b188eeb5e9ba@linaro.org>
Date:   Wed, 7 Sep 2022 13:02:00 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH 3/8] dt-bindings: Add bindings for Tegra234 NVDEC
Content-Language: en-US
To:     Mikko Perttunen <cyndis@kapsi.fi>,
        Thierry Reding <thierry.reding@gmail.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Jonathan Hunter <jonathanh@nvidia.com>
Cc:     Mikko Perttunen <mperttunen@nvidia.com>,
        Ashish Mhetre <amhetre@nvidia.com>,
        Sameer Pujar <spujar@nvidia.com>,
        dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220906132823.2390953-1-cyndis@kapsi.fi>
 <20220906132823.2390953-4-cyndis@kapsi.fi>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20220906132823.2390953-4-cyndis@kapsi.fi>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 06/09/2022 15:28, Mikko Perttunen wrote:
> From: Mikko Perttunen <mperttunen@nvidia.com>
> 
> Update NVDEC bindings for Tegra234. This new engine version only has
> two memory clients, but now requires three clocks, and as a bigger
> change the engine loads firmware from a secure carveout configured by
> the bootloader.
> 
> For the latter, we need to add a phandle to the memory controller
> to query the location of this carveout, and several other properties
> containing offsets into the firmware inside the carveout. These
> properties are intended to be populated through a device tree overlay
> configured at flashing time, so that the values correspond to the
> flashed NVDEC firmware.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  .../gpu/host1x/nvidia,tegra210-nvdec.yaml     | 118 +++++++++++++++---
>  1 file changed, 98 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml
> index 3cf862976448..27128a195b66 100644
> --- a/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml
> +++ b/Documentation/devicetree/bindings/gpu/host1x/nvidia,tegra210-nvdec.yaml
> @@ -24,17 +24,11 @@ properties:
>        - nvidia,tegra210-nvdec
>        - nvidia,tegra186-nvdec
>        - nvidia,tegra194-nvdec
> +      - nvidia,tegra234-nvdec
>  
>    reg:
>      maxItems: 1
>  
> -  clocks:
> -    maxItems: 1
> -
> -  clock-names:
> -    items:
> -      - const: nvdec

Please leave them here with wide constraints (min/maxItems).

> -
>    resets:
>      maxItems: 1
>  
> @@ -50,18 +44,6 @@ properties:
>  
>    dma-coherent: true
>  
> -  interconnects:
> -    items:
> -      - description: DMA read memory client
> -      - description: DMA read 2 memory client
> -      - description: DMA write memory client
> -
> -  interconnect-names:
> -    items:
> -      - const: dma-mem
> -      - const: read-1
> -      - const: write

Please leave them here with wide constraints (min/maxItems).

> -
>    nvidia,host1x-class:
>      description: |
>        Host1x class of the engine, used to specify the targeted engine
> @@ -79,7 +61,103 @@ required:
>    - reset-names
>    - power-domains
>  
> -additionalProperties: false
> +unevaluatedProperties: false

This looks not needed/related.

> +
> +allOf:

Put allOf before additionalProperties:false.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra234-nvdec
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: NVDEC clock
> +            - description: FUSE clock
> +            - description: TSEC_PKA clock
> +        clock-names:
> +          items:
> +            - const: nvdec
> +            - const: fuse
> +            - const: tsec_pka
> +        interconnects:
> +          items:
> +            - description: DMA read memory client
> +            - description: DMA write memory client
> +        interconnect-names:
> +          items:
> +            - const: dma-mem
> +            - const: write
> +        nvidia,memory-controller:
> +          $ref: /schemas/types.yaml#/definitions/phandle
> +          description:
> +            phandle to the memory controller for determining carveout information.

All fields should be defined in top-level. You can disallow them for
other variants, but if the allOf:if:then gets too big, it's a sign to
split the binding.

> +        nvidia,bl-manifest-offset:
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          description:
> +            Offset to bootloader manifest from beginning of firmware. Typically set as
> +            part of a device tree overlay corresponding to flashed firmware.
> +        nvidia,bl-code-offset:
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          description:
> +            Offset to bootloader code section from beginning of firmware. Typically set as
> +            part of a device tree overlay corresponding to flashed firmware.
> +        nvidia,bl-data-offset:
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          description:
> +            Offset to bootloader data section from beginning of firmware. Typically set as
> +            part of a device tree overlay corresponding to flashed firmware.
> +        nvidia,os-manifest-offset:
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          description:
> +            Offset to operating system manifest from beginning of firmware. Typically set as
> +            part of a device tree overlay corresponding to flashed firmware.
> +        nvidia,os-code-offset:
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          description:
> +            Offset to operating system code section from beginning of firmware. Typically set as
> +            part of a device tree overlay corresponding to flashed firmware.
> +        nvidia,os-data-offset:
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          description:
> +            Offset to operating system data section from beginning of firmware. Typically set as
> +            part of a device tree overlay corresponding to flashed firmware.
> +      required:
> +        - nvidia,memory-controller
> +        - nvidia,bl-manifest-offset
> +        - nvidia,bl-code-offset
> +        - nvidia,bl-data-offset
> +        - nvidia,os-manifest-offset
> +        - nvidia,os-code-offset
> +        - nvidia,os-data-offset

blank line

> +  - if:
> +      properties:
> +        compatible:
> +          contains:

Best regards,
Krzysztof