[1/2] dt-bindings: PCI: qcom: Add SC8280XP/SA8540P interconnects

  Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect
paths to the bindings.

Fixes: 76d777ae045e ("dt-bindings: PCI: qcom: Add SC8280XP to binding")
Fixes: 76c4207f4085 ("dt-bindings: PCI: qcom: Add SA8540P to binding")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../devicetree/bindings/pci/qcom,pcie.yaml    | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)
  

On 17/10/2022 07:24, Johan Hovold wrote:
> Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect
> paths to the bindings.
> 
> Fixes: 76d777ae045e ("dt-bindings: PCI: qcom: Add SC8280XP to binding")
> Fixes: 76c4207f4085 ("dt-bindings: PCI: qcom: Add SA8540P to binding")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.yaml    | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> index 22a2aac4c23f..a55434f95edd 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> @@ -62,6 +62,12 @@ properties:
>      minItems: 3
>      maxItems: 12
>  
> +  interconnects:
> +    maxItems: 2
> +
> +  interconnect-names:
> +    maxItems: 2
> +
>    resets:
>      minItems: 1
>      maxItems: 12
> @@ -629,6 +635,25 @@ allOf:
>            items:
>              - const: pci # PCIe core reset
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,pcie-sa8540p
> +              - qcom,pcie-sc8280xp
> +    then:
> +      properties:
> +        interconnects:
> +          maxItems: 2

No need for this.

> +        interconnect-names:
> +          items:
> +            - const: pcie-mem
> +            - const: cpu-pcie
> +      required:
> +        - interconnects
> +        - interconnect-names

else:
  ??

Otherwise, you allow any names for other variants.

Best regards,
Krzysztof
  

On Wed, Oct 19, 2022 at 10:37:31AM -0400, Krzysztof Kozlowski wrote:
> On 17/10/2022 07:24, Johan Hovold wrote:
> > Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect
> > paths to the bindings.
> > 
> > Fixes: 76d777ae045e ("dt-bindings: PCI: qcom: Add SC8280XP to binding")
> > Fixes: 76c4207f4085 ("dt-bindings: PCI: qcom: Add SA8540P to binding")
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  .../devicetree/bindings/pci/qcom,pcie.yaml    | 25 +++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > index 22a2aac4c23f..a55434f95edd 100644
> > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > @@ -62,6 +62,12 @@ properties:
> >      minItems: 3
> >      maxItems: 12
> >  
> > +  interconnects:
> > +    maxItems: 2
> > +
> > +  interconnect-names:
> > +    maxItems: 2
> > +
> >    resets:
> >      minItems: 1
> >      maxItems: 12
> > @@ -629,6 +635,25 @@ allOf:
> >            items:
> >              - const: pci # PCIe core reset
> >  
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,pcie-sa8540p
> > +              - qcom,pcie-sc8280xp
> > +    then:
> > +      properties:
> > +        interconnects:
> > +          maxItems: 2
> 
> No need for this.
> 
> > +        interconnect-names:
> > +          items:
> > +            - const: pcie-mem
> > +            - const: cpu-pcie
> > +      required:
> > +        - interconnects
> > +        - interconnect-names
> 
> else:
>   ??
> 
> Otherwise, you allow any names for other variants.

Are you suggesting something like moving the names to the common
constraints for now:

  interconnects:
    maxItems: 2

  interconnect-names:
    items:
      - const: pcie-mem
      - const: cpu-pcie

and then in the allOf:

  - if:
      properties:
        compatible:
          contains:
            enum:
              - qcom,pcie-sa8540p
              - qcom,pcie-sc8280xp
    then:
      required:
        - interconnects
        - interconnect-names
    else:
      properties:
        interconnects: false
        interconnect-names: false

This way we'd catch anyone adding interconnects to a DTS without first
updating the bindings, but it also seems to go against the idea of
bindings fully describing the hardware by saying that no other platforms
have interconnects (when they actually do even if we don't describe it
just yet).

Or should we do the above but without the else clause to have some
constraints in place on the names at least?

Johan
  

On 20/10/2022 03:57, Johan Hovold wrote:
> On Wed, Oct 19, 2022 at 10:37:31AM -0400, Krzysztof Kozlowski wrote:
>> On 17/10/2022 07:24, Johan Hovold wrote:
>>> Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect
>>> paths to the bindings.
>>>
>>> Fixes: 76d777ae045e ("dt-bindings: PCI: qcom: Add SC8280XP to binding")
>>> Fixes: 76c4207f4085 ("dt-bindings: PCI: qcom: Add SA8540P to binding")
>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>> ---
>>>  .../devicetree/bindings/pci/qcom,pcie.yaml    | 25 +++++++++++++++++++
>>>  1 file changed, 25 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>> index 22a2aac4c23f..a55434f95edd 100644
>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>> @@ -62,6 +62,12 @@ properties:
>>>      minItems: 3
>>>      maxItems: 12
>>>  
>>> +  interconnects:
>>> +    maxItems: 2
>>> +
>>> +  interconnect-names:
>>> +    maxItems: 2
>>> +
>>>    resets:
>>>      minItems: 1
>>>      maxItems: 12
>>> @@ -629,6 +635,25 @@ allOf:
>>>            items:
>>>              - const: pci # PCIe core reset
>>>  
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - qcom,pcie-sa8540p
>>> +              - qcom,pcie-sc8280xp
>>> +    then:
>>> +      properties:
>>> +        interconnects:
>>> +          maxItems: 2
>>
>> No need for this.
>>
>>> +        interconnect-names:
>>> +          items:
>>> +            - const: pcie-mem
>>> +            - const: cpu-pcie
>>> +      required:
>>> +        - interconnects
>>> +        - interconnect-names
>>
>> else:
>>   ??
>>
>> Otherwise, you allow any names for other variants.
> 
> Are you suggesting something like moving the names to the common
> constraints for now:
> 
>   interconnects:
>     maxItems: 2
> 
>   interconnect-names:
>     items:
>       - const: pcie-mem
>       - const: cpu-pcie
> 
> and then in the allOf:
> 
>   - if:
>       properties:
>         compatible:
>           contains:
>             enum:
>               - qcom,pcie-sa8540p
>               - qcom,pcie-sc8280xp
>     then:
>       required:
>         - interconnects
>         - interconnect-names
>     else:
>       properties:
>         interconnects: false
>         interconnect-names: false
> 
> This way we'd catch anyone adding interconnects to a DTS without first
> updating the bindings, but it also seems to go against the idea of
> bindings fully describing the hardware by saying that no other platforms
> have interconnects (when they actually do even if we don't describe it
> just yet).

You can add a comment to the else like "TODO: Not described yet". I
would prefer to have specific but incomplete bindings, instead of loose
one which later might cause people adding whatever names they like.

> Or should we do the above but without the else clause to have some
> constraints in place on the names at least?

This would work as well if you think the names are applicable for other
devices.

Best regards,
Krzysztof
  

On Thu, Oct 20, 2022 at 08:29:02AM -0400, Krzysztof Kozlowski wrote:
> On 20/10/2022 03:57, Johan Hovold wrote:
> > On Wed, Oct 19, 2022 at 10:37:31AM -0400, Krzysztof Kozlowski wrote:
> >> On 17/10/2022 07:24, Johan Hovold wrote:
> >>> Add the missing SC8280XP/SA8540P "pcie-mem" and "cpu-pcie" interconnect
> >>> paths to the bindings.
> >>>
> >>> Fixes: 76d777ae045e ("dt-bindings: PCI: qcom: Add SC8280XP to binding")
> >>> Fixes: 76c4207f4085 ("dt-bindings: PCI: qcom: Add SA8540P to binding")
> >>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >>> ---
> >>>  .../devicetree/bindings/pci/qcom,pcie.yaml    | 25 +++++++++++++++++++
> >>>  1 file changed, 25 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >>> index 22a2aac4c23f..a55434f95edd 100644
> >>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml

> > Are you suggesting something like moving the names to the common
> > constraints for now:
> > 
> >   interconnects:
> >     maxItems: 2
> > 
> >   interconnect-names:
> >     items:
> >       - const: pcie-mem
> >       - const: cpu-pcie
> > 
> > and then in the allOf:
> > 
> >   - if:
> >       properties:
> >         compatible:
> >           contains:
> >             enum:
> >               - qcom,pcie-sa8540p
> >               - qcom,pcie-sc8280xp
> >     then:
> >       required:
> >         - interconnects
> >         - interconnect-names
> >     else:
> >       properties:
> >         interconnects: false
> >         interconnect-names: false
> > 
> > This way we'd catch anyone adding interconnects to a DTS without first
> > updating the bindings, but it also seems to go against the idea of
> > bindings fully describing the hardware by saying that no other platforms
> > have interconnects (when they actually do even if we don't describe it
> > just yet).
> 
> You can add a comment to the else like "TODO: Not described yet". I
> would prefer to have specific but incomplete bindings, instead of loose
> one which later might cause people adding whatever names they like.
> 
> > Or should we do the above but without the else clause to have some
> > constraints in place on the names at least?
> 
> This would work as well if you think the names are applicable for other
> devices.

I think that's a reasonable assumption so I'll go with this alternative.

Thanks!

Johan