diff mbox series

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

Message ID 20240117102526.557006-3-s-vadapalli@ti.com (mailing list archive)
State Changes Requested
Delegated to: Krzysztof WilczyƄski
Headers show
Series Fix and update ti,j721e-pci-* bindings | expand

Commit Message

Siddharth Vadapalli Jan. 17, 2024, 10:25 a.m. UTC
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(+)

Comments

Krzysztof Kozlowski Jan. 17, 2024, 10:35 a.m. UTC | #1
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
Siddharth Vadapalli Jan. 17, 2024, 10:58 a.m. UTC | #2
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?
Krzysztof Kozlowski Jan. 17, 2024, 11 a.m. UTC | #3
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
Siddharth Vadapalli Jan. 17, 2024, 11:15 a.m. UTC | #4
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.
Krzysztof Kozlowski Jan. 17, 2024, 11:19 a.m. UTC | #5
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
Siddharth Vadapalli Jan. 17, 2024, 11:22 a.m. UTC | #6
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
>
Krzysztof Kozlowski Jan. 17, 2024, 11:34 a.m. UTC | #7
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
diff mbox series

Patch

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