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

Jon Hunter
2022-11-14T15:36:38Z

On 14/11/2022 14:23, Krzysztof Kozlowski wrote:
> 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.

Yes I wondered if there was a good way to avoid duplication. It looks
like I cannot have 'maxItems' and 'items' at the top-level, but
obviously I can set 'maxItems' appropriately for each device.

Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml: properties:reg: {'minItems': 4, 'maxItems': 5, '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.'}]} should not be valid under {'required': ['maxItems']}
hint: "maxItems" is not needed with an "items" list

Thanks
Jon

--
nvpublic