[PATCH 0/3] Fix and update ti,j721e-pci-* bindings

[PATCH 0/3] Fix and update ti,j721e-pci-* bindings

[Siddharth Vadapalli]

Hello,

This series fixes the bindings for:
Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
by updating the checks added for validating and enforcing the
"num-lanes" property for different compatibles. In the commits
which introduced and extended the checks for the "num-lanes"
property, the property was not truly validated but only described.
Therefore, the bindings are being updated to actually validate
the "num-lanes" property. While at it, checks for "max-link-speed"
are also being introduced. The intent of the aforementioned changes
is to update the bindings for a new SoC namely TI's J722S SoC which
has a similar PCIe controller to TI's AM64 SoC, but differs from it
in terms of its support for Gen3 link speeds. For this reason, a new
compatible is being added instead of reusing the one available for
AM64 SoC.

Series is based on linux-next tagged next-20240117.

Regards,
Siddharth.

Siddharth Vadapalli (3):
  dt-bindings: PCI: ti,j721e-pci-*: Fix check for num-lanes
  dt-bindings: PCI: ti,j721e-pci-*: Add checks for max-link-speed
  dt-bindings: PCI: ti,j721e-pci-host: Add support for J722S

 .../bindings/pci/ti,j721e-pci-ep.yaml         | 34 +++++++++++---
 .../bindings/pci/ti,j721e-pci-host.yaml       | 47 ++++++++++++++++---
 2 files changed, 67 insertions(+), 14 deletions(-)

-- 
2.34.1

[PATCH 1/3] dt-bindings: PCI: ti,j721e-pci-*: Fix check for num-lanes

[Siddharth Vadapalli]

The existing implementation for validating the "num-lanes" property
based on the compatible(s) doesn't enforce it. Fix it by updating the
checks to handle both single-compatible and multi-compatible cases.

Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes")
Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings")
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 .../bindings/pci/ti,j721e-pci-ep.yaml         | 26 ++++++++++++++-----
 .../bindings/pci/ti,j721e-pci-host.yaml       | 26 ++++++++++++++-----
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
index 97f2579ea908..278e0892f8ac 100644
--- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
@@ -68,8 +68,9 @@ allOf:
   - if:
       properties:
         compatible:
-          enum:
-            - ti,am64-pcie-ep
+          items:
+            - const: ti,am64-pcie-ep
+            - const: ti,j721e-pcie-ep
     then:
       properties:
         num-lanes:
@@ -78,9 +79,9 @@ allOf:
   - if:
       properties:
         compatible:
-          enum:
-            - ti,j7200-pcie-ep
-            - ti,j721e-pcie-ep
+          items:
+            - const: ti,j7200-pcie-ep
+            - const: ti,j721e-pcie-ep
     then:
       properties:
         num-lanes:
@@ -90,8 +91,19 @@ allOf:
   - if:
       properties:
         compatible:
-          enum:
-            - ti,j784s4-pcie-ep
+          items:
+            - const: ti,j721e-pcie-ep
+    then:
+      properties:
+        num-lanes:
+          minimum: 1
+          maximum: 4
+
+  - if:
+      properties:
+        compatible:
+          items:
+            - const: ti,j784s4-pcie-ep
     then:
       properties:
         num-lanes:
diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
index b7a534cef24d..36bcc8cb7896 100644
--- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
@@ -97,8 +97,9 @@ allOf:
   - if:
       properties:
         compatible:
-          enum:
-            - ti,am64-pcie-host
+          items:
+            - const: ti,am64-pcie-host
+            - const: ti,j721e-pcie-host
     then:
       properties:
         num-lanes:
@@ -107,9 +108,9 @@ allOf:
   - if:
       properties:
         compatible:
-          enum:
-            - ti,j7200-pcie-host
-            - ti,j721e-pcie-host
+          items:
+            - const: ti,j7200-pcie-host
+            - const: ti,j721e-pcie-host
     then:
       properties:
         num-lanes:
@@ -119,8 +120,19 @@ allOf:
   - if:
       properties:
         compatible:
-          enum:
-            - ti,j784s4-pcie-host
+          items:
+            - const: ti,j721e-pcie-host
+    then:
+      properties:
+        num-lanes:
+          minimum: 1
+          maximum: 4
+
+  - if:
+      properties:
+        compatible:
+          items:
+            - const: ti,j784s4-pcie-host
     then:
       properties:
         num-lanes:
-- 
2.34.1

[PATCH 2/3] dt-bindings: PCI: ti,j721e-pci-*: Add checks for max-link-speed

[Siddharth Vadapalli]

Extend the existing compatible based checks for validating and enforcing
the "max-link-speed" property.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 .../devicetree/bindings/pci/ti,j721e-pci-ep.yaml          | 8 ++++++++
 .../devicetree/bindings/pci/ti,j721e-pci-host.yaml        | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
index 278e0892f8ac..4839a9574e20 100644
--- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
@@ -73,6 +73,8 @@ allOf:
             - const: ti,j721e-pcie-ep
     then:
       properties:
+        max-link-speed:
+          const: 2
         num-lanes:
           const: 1
 
@@ -84,6 +86,8 @@ allOf:
             - const: ti,j721e-pcie-ep
     then:
       properties:
+        max-link-speed:
+          const: 3
         num-lanes:
           minimum: 1
           maximum: 2
@@ -95,6 +99,8 @@ allOf:
             - const: ti,j721e-pcie-ep
     then:
       properties:
+        max-link-speed:
+          const: 3
         num-lanes:
           minimum: 1
           maximum: 4
@@ -106,6 +112,8 @@ allOf:
             - const: ti,j784s4-pcie-ep
     then:
       properties:
+        max-link-speed:
+          const: 3
         num-lanes:
           minimum: 1
           maximum: 4
diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
index 36bcc8cb7896..005546dc8bd4 100644
--- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
@@ -102,6 +102,8 @@ allOf:
             - const: ti,j721e-pcie-host
     then:
       properties:
+        max-link-speed:
+          const: 2
         num-lanes:
           const: 1
 
@@ -113,6 +115,8 @@ allOf:
             - const: ti,j721e-pcie-host
     then:
       properties:
+        max-link-speed:
+          const: 3
         num-lanes:
           minimum: 1
           maximum: 2
@@ -124,6 +128,8 @@ allOf:
             - const: ti,j721e-pcie-host
     then:
       properties:
+        max-link-speed:
+          const: 3
         num-lanes:
           minimum: 1
           maximum: 4
@@ -135,6 +141,8 @@ allOf:
             - const: ti,j784s4-pcie-host
     then:
       properties:
+        max-link-speed:
+          const: 3
         num-lanes:
           minimum: 1
           maximum: 4
-- 
2.34.1

[PATCH 3/3] dt-bindings: PCI: ti,j721e-pci-host: Add support for J722S SoC

[Siddharth Vadapalli]

TI's J722S SoC has one instance of a Gen3 Single Lane PCIe controller.
The controller on J722S SoC is similar to the one present on TI's AM64
SoC, with the difference being that the controller on AM64 SoC supports
up to Gen2 link speed while the one on J722S SoC supports Gen3 link speed.

Update the bindings with a new compatible for J722S SoC and enforce checks
for "num-lanes" and "max-link-speed".

Technical Reference Manual of J722S SoC: https://www.ti.com/lit/zip/sprujb3

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 .../devicetree/bindings/pci/ti,j721e-pci-host.yaml  | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
index 005546dc8bd4..b7648f7e73c9 100644
--- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
@@ -14,6 +14,7 @@ properties:
   compatible:
     oneOf:
       - const: ti,j721e-pcie-host
+      - const: ti,j722s-pcie-host
       - const: ti,j784s4-pcie-host
       - description: PCIe controller in AM64
         items:
@@ -134,6 +135,18 @@ allOf:
           minimum: 1
           maximum: 4
 
+  - if:
+      properties:
+        compatible:
+          items:
+            - const: ti,j722s-pcie-host
+    then:
+      properties:
+        max-link-speed:
+          const: 3
+        num-lanes:
+          const: 1
+
   - if:
       properties:
         compatible:
-- 
2.34.1

Re: [PATCH 1/3] dt-bindings: PCI: ti,j721e-pci-*: Fix check for num-lanes

[Krzysztof Kozlowski]

On 17/01/2024 11:25, Siddharth Vadapalli wrote:
> The existing implementation for validating the "num-lanes" property
> based on the compatible(s) doesn't enforce it. Fix it by updating the
> checks to handle both single-compatible and multi-compatible cases.
> 
> Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes")
> Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings")
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  .../bindings/pci/ti,j721e-pci-ep.yaml         | 26 ++++++++++++++-----
>  .../bindings/pci/ti,j721e-pci-host.yaml       | 26 ++++++++++++++-----
>  2 files changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
> index 97f2579ea908..278e0892f8ac 100644
> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
> @@ -68,8 +68,9 @@ allOf:
>    - if:
>        properties:
>          compatible:

Missing contains:, instead of your change.

> -          enum:
> -            - ti,am64-pcie-ep
> +          items:
> +            - const: ti,am64-pcie-ep
> +            - const: ti,j721e-pcie-ep

>      then:
>        properties:
>          num-lanes:
> @@ -78,9 +79,9 @@ allOf:
>    - if:
>        properties:
>          compatible:
> -          enum:
> -            - ti,j7200-pcie-ep
> -            - ti,j721e-pcie-ep
> +          items:
> +            - const: ti,j7200-pcie-ep
> +            - const: ti,j721e-pcie-ep

"Ditto

>      then:
>        properties:
>          num-lanes:
> @@ -90,8 +91,19 @@ allOf:
>    - if:
>        properties:
>          compatible:
> -          enum:
> -            - ti,j784s4-pcie-ep
> +          items:
> +            - const: ti,j721e-pcie-ep
> +    then:
> +      properties:
> +        num-lanes:
> +          minimum: 1
> +          maximum: 4
> +
> +  - if:
> +      properties:
> +        compatible:
> +          items:
> +            - const: ti,j784s4-pcie-ep

Why? Previous code was correct.


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: PCI: ti,j721e-pci-*: Add checks for max-link-speed

[Krzysztof Kozlowski]

On 17/01/2024 11:25, Siddharth Vadapalli wrote:
> Extend the existing compatible based checks for validating and enforcing
> the "max-link-speed" property.

Based on what? Driver or hardware? Your entire change suggests you
should just drop it from the binding, because this can be deduced from
compatible.

Best regards,
Krzysztof

Re: [PATCH 3/3] dt-bindings: PCI: ti,j721e-pci-host: Add support for J722S SoC

[Krzysztof Kozlowski]

On 17/01/2024 11:25, Siddharth Vadapalli wrote:
> TI's J722S SoC has one instance of a Gen3 Single Lane PCIe controller.
> The controller on J722S SoC is similar to the one present on TI's AM64
> SoC, with the difference being that the controller on AM64 SoC supports
> up to Gen2 link speed while the one on J722S SoC supports Gen3 link speed.
> 
> Update the bindings with a new compatible for J722S SoC and enforce checks
> for "num-lanes" and "max-link-speed".
> 
> Technical Reference Manual of J722S SoC: https://www.ti.com/lit/zip/sprujb3
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  .../devicetree/bindings/pci/ti,j721e-pci-host.yaml  | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> index 005546dc8bd4..b7648f7e73c9 100644
> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> @@ -14,6 +14,7 @@ properties:
>    compatible:
>      oneOf:
>        - const: ti,j721e-pcie-host
> +      - const: ti,j722s-pcie-host
>        - const: ti,j784s4-pcie-host
>        - description: PCIe controller in AM64
>          items:
> @@ -134,6 +135,18 @@ allOf:
>            minimum: 1
>            maximum: 4
>  
> +  - if:
> +      properties:
> +        compatible:
> +          items:

enum

> +            - const: ti,j722s-pcie-host
> +    then:
> +      properties:
> +        max-link-speed:
> +          const: 3
> +        num-lanes:
> +          const: 1

Similarly to previous patch: What is the point of all this? You have
direct mapping compatible-property, so encode these in the drivers.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: PCI: ti,j721e-pci-*: Fix check for num-lanes

[Siddharth Vadapalli]

Hello Krzysztof,

On 17/01/24 16:04, Krzysztof Kozlowski wrote:
> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>> The existing implementation for validating the "num-lanes" property
>> based on the compatible(s) doesn't enforce it. Fix it by updating the
>> checks to handle both single-compatible and multi-compatible cases.
>>
>> Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes")
>> Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings")
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  .../bindings/pci/ti,j721e-pci-ep.yaml         | 26 ++++++++++++++-----
>>  .../bindings/pci/ti,j721e-pci-host.yaml       | 26 ++++++++++++++-----
>>  2 files changed, 38 insertions(+), 14 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>> index 97f2579ea908..278e0892f8ac 100644
>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>> @@ -68,8 +68,9 @@ allOf:
>>    - if:
>>        properties:
>>          compatible:
> 
> Missing contains:, instead of your change.

I did try the "contains" approach before determining that the implementation in
this patch is more suitable. Please consider the following:

For AM64 SoC the primary compatible is "ti,am64-pcie-ep" and fallback compatible
is "ti,j721e-pcie-ep". For J7200 SoC the primary compatible is
"ti,j7200-pcie-ep" while the fallback compatible is again "ti,j721e-pcie-ep".

Therefore, the device-tree nodes for AM64 and J7200 look like:

AM64:
    compatible = "ti,am64-pcie-ep", "ti,j721e-pcie-ep";
    ...
    num-lanes = 1;

J7200:
    compatible = "ti,j7200-pcie-ep", "ti,j721e-pcie-ep";
    ...
    num-lanes = 4;

This implies that when the check for "num-lanes" is performed on the device-tree
node for PCIe in J7200, the fallback compatible of "ti,j721e-pcie-ep" within the
AM64's "compatible: contains:" check will match the schema and it will check the
existing "num-lanes" being described as "const: 1" against the value in J7200's
PCIe node resulting in a warning. Therefore, using "contains" will result in
errors if the check has to be performed for device-tree nodes with fallback
compatibles. The "items" based approach I have used in this patch ensures that
the schema matches *only* when both the primary and fallback compatible are
present in the device-tree node.

> 
>> -          enum:
>> -            - ti,am64-pcie-ep
>> +          items:
>> +            - const: ti,am64-pcie-ep
>> +            - const: ti,j721e-pcie-ep
> 
>>      then:
>>        properties:
>>          num-lanes:
>> @@ -78,9 +79,9 @@ allOf:
>>    - if:
>>        properties:
>>          compatible:
>> -          enum:
>> -            - ti,j7200-pcie-ep
>> -            - ti,j721e-pcie-ep
>> +          items:
>> +            - const: ti,j7200-pcie-ep
>> +            - const: ti,j721e-pcie-ep
> 
> "Ditto

Same explanation as above.

> 
>>      then:
>>        properties:
>>          num-lanes:
>> @@ -90,8 +91,19 @@ allOf:
>>    - if:
>>        properties:
>>          compatible:
>> -          enum:
>> -            - ti,j784s4-pcie-ep
>> +          items:
>> +            - const: ti,j721e-pcie-ep
>> +    then:
>> +      properties:
>> +        num-lanes:
>> +          minimum: 1
>> +          maximum: 4
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          items:
>> +            - const: ti,j784s4-pcie-ep
> 
> Why? Previous code was correct.

Though I used "patience diff", for some reason the addition of
"ti,j721e-pcie-ep" in the check has been treated as the removal of
"ti,j784s4-pcie-ep" first followed by adding the same later for generating the
diff in this patch. The diff above is equivalent to the addition of:

  - if:
      properties:
        compatible:
          items:
            - const: ti,j721e-pcie-ep
    then:
      properties:
        num-lanes:
          minimum: 1
          maximum: 4

-- 
Regards,
Siddharth.

Re: [PATCH 1/3] dt-bindings: PCI: ti,j721e-pci-*: Fix check for num-lanes

[Krzysztof Kozlowski]

On 17/01/2024 11:47, Siddharth Vadapalli wrote:
> Hello Krzysztof,
> 
> On 17/01/24 16:04, Krzysztof Kozlowski wrote:
>> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>>> The existing implementation for validating the "num-lanes" property
>>> based on the compatible(s) doesn't enforce it. Fix it by updating the
>>> checks to handle both single-compatible and multi-compatible cases.
>>>
>>> Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes")
>>> Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings")
>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>> ---
>>>  .../bindings/pci/ti,j721e-pci-ep.yaml         | 26 ++++++++++++++-----
>>>  .../bindings/pci/ti,j721e-pci-host.yaml       | 26 ++++++++++++++-----
>>>  2 files changed, 38 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>>> index 97f2579ea908..278e0892f8ac 100644
>>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>>> @@ -68,8 +68,9 @@ allOf:
>>>    - if:
>>>        properties:
>>>          compatible:
>>
>> Missing contains:, instead of your change.
> 
> I did try the "contains" approach before determining that the implementation in
> this patch is more suitable. Please consider the following:
> 
> For AM64 SoC the primary compatible is "ti,am64-pcie-ep" and fallback compatible
> is "ti,j721e-pcie-ep". For J7200 SoC the primary compatible is
> "ti,j7200-pcie-ep" while the fallback compatible is again "ti,j721e-pcie-ep".
> 
> Therefore, the device-tree nodes for AM64 and J7200 look like:
> 
> AM64:
>     compatible = "ti,am64-pcie-ep", "ti,j721e-pcie-ep";
>     ...
>     num-lanes = 1;
> 
> J7200:
>     compatible = "ti,j7200-pcie-ep", "ti,j721e-pcie-ep";
>     ...
>     num-lanes = 4;
> 
> This implies that when the check for "num-lanes" is performed on the device-tree
> node for PCIe in J7200, the fallback compatible of "ti,j721e-pcie-ep" within the
> AM64's "compatible: contains:" check will match the schema and it will check the
> existing "num-lanes" being described as "const: 1" against the value in J7200's
> PCIe node resulting in a warning. 

What warning? What did you put to contains?

> Therefore, using "contains" will result in
> errors if the check has to be performed for device-tree nodes with fallback
> compatibles. The "items" based approach I have used in this patch ensures that
> the schema matches *only* when both the primary and fallback compatible are
> present in the device-tree node.

Long message, but I don't understand it. Why this binding is different
than all others which rely on contains?

>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          items:
>>> +            - const: ti,j784s4-pcie-ep
>>
>> Why? Previous code was correct.
> 
> Though I used "patience diff", for some reason the addition of
> "ti,j721e-pcie-ep" in the check has been treated as the removal of
> "ti,j784s4-pcie-ep" first followed by adding the same later for generating the
> diff in this patch. The diff above is equivalent to the addition of:

No, why do you change existing code? It is correct.


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: PCI: ti,j721e-pci-*: Add checks for max-link-speed

[Siddharth Vadapalli]

On 17/01/24 16:05, Krzysztof Kozlowski wrote:
> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>> Extend the existing compatible based checks for validating and enforcing
>> the "max-link-speed" property.
> 
> Based on what? Driver or hardware? Your entire change suggests you

Hardware. The PCIe controller on AM64 SoC supports up to Gen2 link speed while
the PCIe controllers on other SoCs support Gen3 link speed.

> should just drop it from the binding, because this can be deduced from
> compatible.

Could you please clarify? Isn't the addition of the checks for "max-link-speed"
identical to the checks which were added for "num-lanes", both of which are
Hardware specific?

-- 
Regards,
Siddharth.

Re: [PATCH 2/3] dt-bindings: PCI: ti,j721e-pci-*: Add checks for max-link-speed

[Krzysztof Kozlowski]

On 17/01/2024 11:58, Siddharth Vadapalli wrote:
> On 17/01/24 16:05, Krzysztof Kozlowski wrote:
>> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>>> Extend the existing compatible based checks for validating and enforcing
>>> the "max-link-speed" property.
>>
>> Based on what? Driver or hardware? Your entire change suggests you
> 
> Hardware. The PCIe controller on AM64 SoC supports up to Gen2 link speed while
> the PCIe controllers on other SoCs support Gen3 link speed.
> 
>> should just drop it from the binding, because this can be deduced from
>> compatible.
> 
> Could you please clarify? Isn't the addition of the checks for "max-link-speed"
> identical to the checks which were added for "num-lanes", both of which are
> Hardware specific?

Compatible defines these values, at least what it looks like from the patch.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: PCI: ti,j721e-pci-*: Fix check for num-lanes

[Siddharth Vadapalli]

On 17/01/24 16:23, Krzysztof Kozlowski wrote:
> On 17/01/2024 11:47, Siddharth Vadapalli wrote:
>> Hello Krzysztof,
>>
>> On 17/01/24 16:04, Krzysztof Kozlowski wrote:
>>> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>>>> The existing implementation for validating the "num-lanes" property
>>>> based on the compatible(s) doesn't enforce it. Fix it by updating the
>>>> checks to handle both single-compatible and multi-compatible cases.
>>>>
>>>> Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes")
>>>> Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings")
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>> ---
>>>>  .../bindings/pci/ti,j721e-pci-ep.yaml         | 26 ++++++++++++++-----
>>>>  .../bindings/pci/ti,j721e-pci-host.yaml       | 26 ++++++++++++++-----
>>>>  2 files changed, 38 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>>>> index 97f2579ea908..278e0892f8ac 100644
>>>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>>>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>>>> @@ -68,8 +68,9 @@ allOf:
>>>>    - if:
>>>>        properties:
>>>>          compatible:
>>>
>>> Missing contains:, instead of your change.
>>
>> I did try the "contains" approach before determining that the implementation in
>> this patch is more suitable. Please consider the following:
>>
>> For AM64 SoC the primary compatible is "ti,am64-pcie-ep" and fallback compatible
>> is "ti,j721e-pcie-ep". For J7200 SoC the primary compatible is
>> "ti,j7200-pcie-ep" while the fallback compatible is again "ti,j721e-pcie-ep".
>>
>> Therefore, the device-tree nodes for AM64 and J7200 look like:
>>
>> AM64:
>>     compatible = "ti,am64-pcie-ep", "ti,j721e-pcie-ep";
>>     ...
>>     num-lanes = 1;
>>
>> J7200:
>>     compatible = "ti,j7200-pcie-ep", "ti,j721e-pcie-ep";
>>     ...
>>     num-lanes = 4;
>>
>> This implies that when the check for "num-lanes" is performed on the device-tree
>> node for PCIe in J7200, the fallback compatible of "ti,j721e-pcie-ep" within the
>> AM64's "compatible: contains:" check will match the schema and it will check the
>> existing "num-lanes" being described as "const: 1" against the value in J7200's
>> PCIe node resulting in a warning. 
> 
> What warning? What did you put to contains?

The warning is:
num-lanes: expected value is 1
which it has determined due to the presence of "ti,j721e-pcie-ep" in the first
check which is only applicable to AM64. The shared fallback compatible here is
responsible for incorrect checks when using "contains".

Using "contains", the check for "num-lanes" with "const: 1" corresponding to
AM64 ends up validating against the device-tree node for J7200 since the
fallback compatible "ti,j721e-pcie-ep" is "contained" in the list of compatibles
present in the device-tree node. That shouldn't be the case which is why "items"
is used in this patch to get an exact match.

> 
>> Therefore, using "contains" will result in
>> errors if the check has to be performed for device-tree nodes with fallback
>> compatibles. The "items" based approach I have used in this patch ensures that
>> the schema matches *only* when both the primary and fallback compatible are
>> present in the device-tree node.
> 
> Long message, but I don't understand it. Why this binding is different
> than all others which rely on contains?

This binding is different because of the existence of a shared fallback
compatible and a shared property being evaluated. In other bindings which use
contains, either there isn't a shared fallback compatible, or the property which
is present in device-tree nodes which have the shared fallback compatible isn't
evaluated.

In brief, with the existing device-tree, without any changes, adding "contains"
will throw warnings due to the incorrect schema matching for validating the
"num-lanes" property.

> 
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          items:
>>>> +            - const: ti,j784s4-pcie-ep
>>>
>>> Why? Previous code was correct.
>>
>> Though I used "patience diff", for some reason the addition of
>> "ti,j721e-pcie-ep" in the check has been treated as the removal of
>> "ti,j784s4-pcie-ep" first followed by adding the same later for generating the
>> diff in this patch. The diff above is equivalent to the addition of:
> 
> No, why do you change existing code? It is correct.

Either a "contains" or an "items" is required to evaluate the "num-lanes"
property and neither of them are present in the existing code.

-- 
Regards,
Siddharth.

Re: [PATCH 2/3] dt-bindings: PCI: ti,j721e-pci-*: Add checks for max-link-speed

[Siddharth Vadapalli]

On 17/01/24 16:30, Krzysztof Kozlowski wrote:
> On 17/01/2024 11:58, Siddharth Vadapalli wrote:
>> On 17/01/24 16:05, Krzysztof Kozlowski wrote:
>>> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>>>> Extend the existing compatible based checks for validating and enforcing
>>>> the "max-link-speed" property.
>>>
>>> Based on what? Driver or hardware? Your entire change suggests you
>>
>> Hardware. The PCIe controller on AM64 SoC supports up to Gen2 link speed while
>> the PCIe controllers on other SoCs support Gen3 link speed.
>>
>>> should just drop it from the binding, because this can be deduced from
>>> compatible.
>>
>> Could you please clarify? Isn't the addition of the checks for "max-link-speed"
>> identical to the checks which were added for "num-lanes", both of which are
>> Hardware specific?
> 
> Compatible defines these values, at least what it looks like from the patch.

In this patch, I have added checks for the "max-link-speed" property in the same
section that "num-lanes" is being evaluated. The values for "max-link-speed" are
based on the Hardware support and this patch is validating the "max-link-speed"
property in the device-tree nodes for the devices against the Hardware supported
values which this patch is adding in the corresponding section. Kindly let me
know if I misunderstood what you meant to convey.

-- 
Regards,
Siddharth.

Re: [PATCH 1/3] dt-bindings: PCI: ti,j721e-pci-*: Fix check for num-lanes

[Krzysztof Kozlowski]

On 17/01/2024 12:11, Siddharth Vadapalli wrote:
> 
> 
> On 17/01/24 16:23, Krzysztof Kozlowski wrote:
>> On 17/01/2024 11:47, Siddharth Vadapalli wrote:
>>> Hello Krzysztof,
>>>
>>> On 17/01/24 16:04, Krzysztof Kozlowski wrote:
>>>> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>>>>> The existing implementation for validating the "num-lanes" property
>>>>> based on the compatible(s) doesn't enforce it. Fix it by updating the
>>>>> checks to handle both single-compatible and multi-compatible cases.
>>>>>
>>>>> Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes")
>>>>> Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings")
>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>> ---
>>>>>  .../bindings/pci/ti,j721e-pci-ep.yaml         | 26 ++++++++++++++-----
>>>>>  .../bindings/pci/ti,j721e-pci-host.yaml       | 26 ++++++++++++++-----
>>>>>  2 files changed, 38 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>>>>> index 97f2579ea908..278e0892f8ac 100644
>>>>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>>>>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>>>>> @@ -68,8 +68,9 @@ allOf:
>>>>>    - if:
>>>>>        properties:
>>>>>          compatible:
>>>>
>>>> Missing contains:, instead of your change.
>>>
>>> I did try the "contains" approach before determining that the implementation in
>>> this patch is more suitable. Please consider the following:
>>>
>>> For AM64 SoC the primary compatible is "ti,am64-pcie-ep" and fallback compatible
>>> is "ti,j721e-pcie-ep". For J7200 SoC the primary compatible is
>>> "ti,j7200-pcie-ep" while the fallback compatible is again "ti,j721e-pcie-ep".
>>>
>>> Therefore, the device-tree nodes for AM64 and J7200 look like:
>>>
>>> AM64:
>>>     compatible = "ti,am64-pcie-ep", "ti,j721e-pcie-ep";
>>>     ...
>>>     num-lanes = 1;
>>>
>>> J7200:
>>>     compatible = "ti,j7200-pcie-ep", "ti,j721e-pcie-ep";
>>>     ...
>>>     num-lanes = 4;
>>>
>>> This implies that when the check for "num-lanes" is performed on the device-tree
>>> node for PCIe in J7200, the fallback compatible of "ti,j721e-pcie-ep" within the
>>> AM64's "compatible: contains:" check will match the schema and it will check the
>>> existing "num-lanes" being described as "const: 1" against the value in J7200's
>>> PCIe node resulting in a warning. 
>>
>> What warning? What did you put to contains?
> 
> The warning is:
> num-lanes: expected value is 1
> which it has determined due to the presence of "ti,j721e-pcie-ep" in the first
> check which is only applicable to AM64. The shared fallback compatible here is
> responsible for incorrect checks when using "contains".
> 
> Using "contains", the check for "num-lanes" with "const: 1" corresponding to
> AM64 ends up validating against the device-tree node for J7200 since the
> fallback compatible "ti,j721e-pcie-ep" is "contained" in the list of compatibles

Why do you put fallback to contains? It does not make sense. You want to
check for containing the device compatible.

> present in the device-tree node. That shouldn't be the case which is why "items"
> is used in this patch to get an exact match.
> 
>>
>>> Therefore, using "contains" will result in
>>> errors if the check has to be performed for device-tree nodes with fallback
>>> compatibles. The "items" based approach I have used in this patch ensures that
>>> the schema matches *only* when both the primary and fallback compatible are
>>> present in the device-tree node.
>>
>> Long message, but I don't understand it. Why this binding is different
>> than all others which rely on contains?
> 
> This binding is different because of the existence of a shared fallback

Many other bindings are the same.

> compatible and a shared property being evaluated. In other bindings which use
> contains, either there isn't a shared fallback compatible, or the property which

No, we do not talk about such bindings. We talk about fallbacks. Using
contains for other cases is redundant, so why even bringing them up here?

> is present in device-tree nodes which have the shared fallback compatible isn't
> evaluated.>
> In brief, with the existing device-tree, without any changes, adding "contains"
> will throw warnings due to the incorrect schema matching for validating the
> "num-lanes" property.

? What?
> 
>>
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          items:
>>>>> +            - const: ti,j784s4-pcie-ep
>>>>
>>>> Why? Previous code was correct.
>>>
>>> Though I used "patience diff", for some reason the addition of
>>> "ti,j721e-pcie-ep" in the check has been treated as the removal of
>>> "ti,j784s4-pcie-ep" first followed by adding the same later for generating the
>>> diff in this patch. The diff above is equivalent to the addition of:
>>
>> No, why do you change existing code? It is correct.
> 
> Either a "contains" or an "items" is required to evaluate the "num-lanes"
> property and neither of them are present in the existing code.

No, it's not. Your code is equivalent to old handling of j784s4.
Contains is totally irrelevant here.

NAK.

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: PCI: ti,j721e-pci-*: Add checks for max-link-speed

[Krzysztof Kozlowski]

On 17/01/2024 12:15, Siddharth Vadapalli wrote:
> 
> 
> On 17/01/24 16:30, Krzysztof Kozlowski wrote:
>> On 17/01/2024 11:58, Siddharth Vadapalli wrote:
>>> On 17/01/24 16:05, Krzysztof Kozlowski wrote:
>>>> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>>>>> Extend the existing compatible based checks for validating and enforcing
>>>>> the "max-link-speed" property.
>>>>
>>>> Based on what? Driver or hardware? Your entire change suggests you
>>>
>>> Hardware. The PCIe controller on AM64 SoC supports up to Gen2 link speed while
>>> the PCIe controllers on other SoCs support Gen3 link speed.
>>>
>>>> should just drop it from the binding, because this can be deduced from
>>>> compatible.
>>>
>>> Could you please clarify? Isn't the addition of the checks for "max-link-speed"
>>> identical to the checks which were added for "num-lanes", both of which are
>>> Hardware specific?
>>
>> Compatible defines these values, at least what it looks like from the patch.
> 
> In this patch, I have added checks for the "max-link-speed" property in the same
> section that "num-lanes" is being evaluated. 

I know what you did in patch. I read it.

> The values for "max-link-speed" are
> based on the Hardware support and this patch is validating the "max-link-speed"
> property in the device-tree nodes for the devices against the Hardware supported
> values which this patch is adding in the corresponding section. Kindly let me
> know if I misunderstood what you meant to convey.

Nothing of this is relevant.

I used two entirely different wordings for this and you still don't get
it, so I don't know if I have third one.

Maybe this:
Move it to driver match data.

So three entirely different wordings for the same. I don't have fourth...

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: PCI: ti,j721e-pci-*: Add checks for max-link-speed

[Siddharth Vadapalli]

On 17/01/24 16:49, Krzysztof Kozlowski wrote:
> On 17/01/2024 12:15, Siddharth Vadapalli wrote:
>>
>>
>> On 17/01/24 16:30, Krzysztof Kozlowski wrote:
>>> On 17/01/2024 11:58, Siddharth Vadapalli wrote:
>>>> On 17/01/24 16:05, Krzysztof Kozlowski wrote:
>>>>> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>>>>>> Extend the existing compatible based checks for validating and enforcing
>>>>>> the "max-link-speed" property.
>>>>>
>>>>> Based on what? Driver or hardware? Your entire change suggests you
>>>>
>>>> Hardware. The PCIe controller on AM64 SoC supports up to Gen2 link speed while
>>>> the PCIe controllers on other SoCs support Gen3 link speed.
>>>>
>>>>> should just drop it from the binding, because this can be deduced from
>>>>> compatible.
>>>>
>>>> Could you please clarify? Isn't the addition of the checks for "max-link-speed"
>>>> identical to the checks which were added for "num-lanes", both of which are
>>>> Hardware specific?
>>>
>>> Compatible defines these values, at least what it looks like from the patch.
>>
>> In this patch, I have added checks for the "max-link-speed" property in the same
>> section that "num-lanes" is being evaluated. 
> 
> I know what you did in patch. I read it.
> 
>> The values for "max-link-speed" are
>> based on the Hardware support and this patch is validating the "max-link-speed"
>> property in the device-tree nodes for the devices against the Hardware supported
>> values which this patch is adding in the corresponding section. Kindly let me
>> know if I misunderstood what you meant to convey.
> 
> Nothing of this is relevant.
> 
> I used two entirely different wordings for this and you still don't get
> it, so I don't know if I have third one.
> 
> Maybe this:
> Move it to driver match data.

Ok. I will drop the checks for "max-link-speed" and move them to the driver. But
I wonder why the checks for "num-lanes" are needed in the first place when they
could be in the driver as well.

> 
> So three entirely different wordings for the same. I don't have fourth...
> 
> Best regards,
> Krzysztof
> 

-- 
Regards,
Siddharth.

Re: [PATCH 3/3] dt-bindings: PCI: ti,j721e-pci-host: Add support for J722S SoC

[Siddharth Vadapalli]

On 17/01/24 16:06, Krzysztof Kozlowski wrote:
> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>> TI's J722S SoC has one instance of a Gen3 Single Lane PCIe controller.
>> The controller on J722S SoC is similar to the one present on TI's AM64
>> SoC, with the difference being that the controller on AM64 SoC supports
>> up to Gen2 link speed while the one on J722S SoC supports Gen3 link speed.
>>
>> Update the bindings with a new compatible for J722S SoC and enforce checks
>> for "num-lanes" and "max-link-speed".
>>
>> Technical Reference Manual of J722S SoC: https://www.ti.com/lit/zip/sprujb3
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  .../devicetree/bindings/pci/ti,j721e-pci-host.yaml  | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> index 005546dc8bd4..b7648f7e73c9 100644
>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> @@ -14,6 +14,7 @@ properties:
>>    compatible:
>>      oneOf:
>>        - const: ti,j721e-pcie-host
>> +      - const: ti,j722s-pcie-host
>>        - const: ti,j784s4-pcie-host
>>        - description: PCIe controller in AM64
>>          items:
>> @@ -134,6 +135,18 @@ allOf:
>>            minimum: 1
>>            maximum: 4
>>  
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          items:
> 
> enum
> 
>> +            - const: ti,j722s-pcie-host
>> +    then:
>> +      properties:
>> +        max-link-speed:
>> +          const: 3
>> +        num-lanes:
>> +          const: 1
> 
> Similarly to previous patch: What is the point of all this? You have
> direct mapping compatible-property, so encode these in the drivers.

Ok. I will drop patches 1 and 2 of this series and only post v2 of this patch
for adding a new compatible for J722S SoC without any checks for
"max-link-speed" or "num-lanes" for the new compatible.

-- 
Regards,
Siddharth.

Re: [PATCH 2/3] dt-bindings: PCI: ti,j721e-pci-*: Add checks for max-link-speed

[Krzysztof Kozlowski]

On 17/01/2024 12:22, Siddharth Vadapalli wrote:
> 
> 
> On 17/01/24 16:49, Krzysztof Kozlowski wrote:
>> On 17/01/2024 12:15, Siddharth Vadapalli wrote:
>>>
>>>
>>> On 17/01/24 16:30, Krzysztof Kozlowski wrote:
>>>> On 17/01/2024 11:58, Siddharth Vadapalli wrote:
>>>>> On 17/01/24 16:05, Krzysztof Kozlowski wrote:
>>>>>> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>>>>>>> Extend the existing compatible based checks for validating and enforcing
>>>>>>> the "max-link-speed" property.
>>>>>>
>>>>>> Based on what? Driver or hardware? Your entire change suggests you
>>>>>
>>>>> Hardware. The PCIe controller on AM64 SoC supports up to Gen2 link speed while
>>>>> the PCIe controllers on other SoCs support Gen3 link speed.
>>>>>
>>>>>> should just drop it from the binding, because this can be deduced from
>>>>>> compatible.
>>>>>
>>>>> Could you please clarify? Isn't the addition of the checks for "max-link-speed"
>>>>> identical to the checks which were added for "num-lanes", both of which are
>>>>> Hardware specific?
>>>>
>>>> Compatible defines these values, at least what it looks like from the patch.
>>>
>>> In this patch, I have added checks for the "max-link-speed" property in the same
>>> section that "num-lanes" is being evaluated. 
>>
>> I know what you did in patch. I read it.
>>
>>> The values for "max-link-speed" are
>>> based on the Hardware support and this patch is validating the "max-link-speed"
>>> property in the device-tree nodes for the devices against the Hardware supported
>>> values which this patch is adding in the corresponding section. Kindly let me
>>> know if I misunderstood what you meant to convey.
>>
>> Nothing of this is relevant.
>>
>> I used two entirely different wordings for this and you still don't get
>> it, so I don't know if I have third one.
>>
>> Maybe this:
>> Move it to driver match data.
> 
> Ok. I will drop the checks for "max-link-speed" and move them to the driver. But
> I wonder why the checks for "num-lanes" are needed in the first place when they
> could be in the driver as well.

Yes, that's why I commented in your next patch.

Best regards,
Krzysztof

Re: [PATCH 3/3] dt-bindings: PCI: ti,j721e-pci-host: Add support for J722S SoC

[Krzysztof Kozlowski]

On 17/01/2024 12:24, Siddharth Vadapalli wrote:
> 
> 
> On 17/01/24 16:06, Krzysztof Kozlowski wrote:
>> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>>> TI's J722S SoC has one instance of a Gen3 Single Lane PCIe controller.
>>> The controller on J722S SoC is similar to the one present on TI's AM64
>>> SoC, with the difference being that the controller on AM64 SoC supports
>>> up to Gen2 link speed while the one on J722S SoC supports Gen3 link speed.
>>>
>>> Update the bindings with a new compatible for J722S SoC and enforce checks
>>> for "num-lanes" and "max-link-speed".
>>>
>>> Technical Reference Manual of J722S SoC: https://www.ti.com/lit/zip/sprujb3
>>>
>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>> ---
>>>  .../devicetree/bindings/pci/ti,j721e-pci-host.yaml  | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>>> index 005546dc8bd4..b7648f7e73c9 100644
>>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>>> @@ -14,6 +14,7 @@ properties:
>>>    compatible:
>>>      oneOf:
>>>        - const: ti,j721e-pcie-host
>>> +      - const: ti,j722s-pcie-host
>>>        - const: ti,j784s4-pcie-host
>>>        - description: PCIe controller in AM64
>>>          items:
>>> @@ -134,6 +135,18 @@ allOf:
>>>            minimum: 1
>>>            maximum: 4
>>>  
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          items:
>>
>> enum
>>
>>> +            - const: ti,j722s-pcie-host
>>> +    then:
>>> +      properties:
>>> +        max-link-speed:
>>> +          const: 3
>>> +        num-lanes:
>>> +          const: 1
>>
>> Similarly to previous patch: What is the point of all this? You have
>> direct mapping compatible-property, so encode these in the drivers.
> 
> Ok. I will drop patches 1 and 2 of this series and only post v2 of this patch
> for adding a new compatible for J722S SoC without any checks for
> "max-link-speed" or "num-lanes" for the new compatible.

Without driver change? So how does it solve my comment. I want to move
all these unnecessary properties to the driver.

Best regards,
Krzysztof

Re: [PATCH 3/3] dt-bindings: PCI: ti,j721e-pci-host: Add support for J722S SoC

[Siddharth Vadapalli]

On 17/01/24 17:05, Krzysztof Kozlowski wrote:
> On 17/01/2024 12:24, Siddharth Vadapalli wrote:
>>
>>
>> On 17/01/24 16:06, Krzysztof Kozlowski wrote:
>>> On 17/01/2024 11:25, Siddharth Vadapalli wrote:
>>>> TI's J722S SoC has one instance of a Gen3 Single Lane PCIe controller.
>>>> The controller on J722S SoC is similar to the one present on TI's AM64
>>>> SoC, with the difference being that the controller on AM64 SoC supports
>>>> up to Gen2 link speed while the one on J722S SoC supports Gen3 link speed.
>>>>
>>>> Update the bindings with a new compatible for J722S SoC and enforce checks
>>>> for "num-lanes" and "max-link-speed".
>>>>
>>>> Technical Reference Manual of J722S SoC: https://www.ti.com/lit/zip/sprujb3
>>>>
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>> ---
>>>>  .../devicetree/bindings/pci/ti,j721e-pci-host.yaml  | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>>>> index 005546dc8bd4..b7648f7e73c9 100644
>>>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>>>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>>>> @@ -14,6 +14,7 @@ properties:
>>>>    compatible:
>>>>      oneOf:
>>>>        - const: ti,j721e-pcie-host
>>>> +      - const: ti,j722s-pcie-host
>>>>        - const: ti,j784s4-pcie-host
>>>>        - description: PCIe controller in AM64
>>>>          items:
>>>> @@ -134,6 +135,18 @@ allOf:
>>>>            minimum: 1
>>>>            maximum: 4
>>>>  
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          items:
>>>
>>> enum
>>>
>>>> +            - const: ti,j722s-pcie-host
>>>> +    then:
>>>> +      properties:
>>>> +        max-link-speed:
>>>> +          const: 3
>>>> +        num-lanes:
>>>> +          const: 1
>>>
>>> Similarly to previous patch: What is the point of all this? You have
>>> direct mapping compatible-property, so encode these in the drivers.
>>
>> Ok. I will drop patches 1 and 2 of this series and only post v2 of this patch
>> for adding a new compatible for J722S SoC without any checks for
>> "max-link-speed" or "num-lanes" for the new compatible.
> 
> Without driver change? So how does it solve my comment. I want to move
> all these unnecessary properties to the driver.

Driver support for verifying "num-lanes" is already present. I meant to say that
I will drop the checks in bindings in the v2 patch since "num-lanes" is verified
in driver too. However, "max-link-speed" is not verified either in the driver or
in the bindings prior to this series.

-- 
Regards,
Siddharth.