mbox series

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

Message ID 20240117102526.557006-1-s-vadapalli@ti.com
Headers show
Series Fix and update ti,j721e-pci-* bindings | expand

Message

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

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:19 a.m. UTC | #3 
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:24 a.m. UTC | #4 
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.
Krzysztof Kozlowski Jan. 17, 2024, 11:35 a.m. UTC | #5 
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