Re: [PATCH V2 1/2] dt-bindings: PCI: tegra234: Add ECAM support

Krzysztof Kozlowski
2022-11-14T14:23:38Z

On 14/11/2022 15:09, Jon Hunter wrote:
> From: Vidya Sagar <vidyas@nvidia.com>
>
> Add support for ECAM aperture that is only supported for Tegra234
> devices.
>
> Co-developed-by: Vidya Sagar <vidyas@nvidia.com>
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> Co-developed-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> Changes since V1:
> - Restricted the ECAM aperture to only Tegra234 devices that support it.
>
> .../bindings/pci/nvidia,tegra194-pcie.yaml | 76 +++++++++++++++----
> .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +-
> 2 files changed, 62 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml
> index 75da3e8eecb9..7ae0f37f5364 100644
> --- a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml
> @@ -27,21 +27,12 @@ properties:
> - nvidia,tegra234-pcie
>
> reg:
> - items:
> - - description: controller's application logic registers
> - - description: configuration registers
> - - description: iATU and DMA registers. This is where the iATU (internal
> - Address Translation Unit) registers of the PCIe core are made
> - available for software access.
> - - description: aperture where the Root Port's own configuration
> - registers are available.
> + minItems: 4
> + maxItems: 5
>
> reg-names:
> - items:
> - - const: appl
> - - const: config
> - - const: atu_dma
> - - const: dbi
> + minItems: 4
> + maxItems: 5
>
> interrupts:
> items:
> @@ -202,6 +193,60 @@ properties:
>
> allOf:
> - $ref: /schemas/pci/snps,dw-pcie.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - nvidia,tegra194-pcie
> + then:
> + properties:
> + reg:
> + minItems: 4
> + maxItems: 4

How you wrote it, you do not need min/maxItems here, because you have
items below. However see further comment.

> + items:
> + - description: controller's application logic registers
> + - description: configuration registers
> + - description: iATU and DMA registers. This is where the iATU (internal
> + Address Translation Unit) registers of the PCIe core are made
> + available for software access.
> + - description: aperture where the Root Port's own configuration
> + registers are available.
> + reg-names:
> + items:
> + - const: appl
> + - const: config
> + - const: atu_dma
> + - const: dbi
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - nvidia,tegra234-pcie
> + then:
> + properties:
> + reg:
> + minItems: 5
> + maxItems: 5

Similar issue.

> + items:
> + - description: controller's application logic registers
> + - description: configuration registers
> + - description: iATU and DMA registers. This is where the iATU (internal
> + Address Translation Unit) registers of the PCIe core are made
> + available for software access.
> + - description: aperture where the Root Port's own configuration
> + registers are available.
> + - description: aperture to access the configuration space through ECAM.

This is unnecessarily duplicated. You can keep the descriptions of items
and reg-names items in top level (with min 4 and max 5) and restrict
maxItems for 194 and minItems for 234 here.

> + reg-names:
> + items:
> + - const: appl
> + - const: config
> + - const: atu_dma
> + - const: dbi
> + - const: ecam
> +

No need for blank line.
>
> unevaluatedProperties: false
>

Best regards,
Krzysztof