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

[Jon Hunter <jonathanh@nvidia.com>]

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