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

Jon Hunter
2022-11-14T15:58:05Z

On 14/11/2022 15:36, Jon Hunter wrote:
>
> 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.

Ah I get it. The 'items' list is defining the max. I have fixed up and
sent out a new revision.

Jon

--
nvpublic