TI's J721e SoC supports additional PHY modes like QSGMII and SGMII
that are not supported on earlier SoCs. Add a compatible for it.
Extend ti,qsgmii-main-ports property to support selection of upto
two main ports at once across the two QSGMII interfaces.
Signed-off-by: Siddharth Vadapalli <[email protected]>
---
.../bindings/phy/ti,phy-gmii-sel.yaml | 52 ++++++++++++++++---
1 file changed, 46 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
index da7cac537e15..1e19efab018b 100644
--- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
@@ -54,6 +54,7 @@ properties:
- ti,dm814-phy-gmii-sel
- ti,am654-phy-gmii-sel
- ti,j7200-cpsw5g-phy-gmii-sel
+ - ti,j721e-cpsw9g-phy-gmii-sel
reg:
maxItems: 1
@@ -65,12 +66,19 @@ properties:
description: |
Required only for QSGMII mode. Array to select the port for
QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
- ports automatically. Any one of the 4 CPSW5G ports can act as the
- main port with the rest of them being the QSGMII_SUB ports.
- maxItems: 1
- items:
- minimum: 1
- maximum: 4
+ ports automatically. For J7200 CPSW5G with the compatible:
+ ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
+ array of only one element, which is the port number ranging from
+ 1 to 4. For J721e CPSW9G with the compatible:
+ ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
+ of two elements, which corresponds to two potential QSGMII main
+ ports. The first element and second element of the array can both
+ range from 1 to 8 each, corresponding to two QSGMII main ports.
+ For J721e CPSW9G, to configure port 2 as the first QSGMII main
+ port and port 7 as the second QSGMII main port, we specify:
+ ti,qsgmii-main-ports = <2>, <7>;
+ If only one QSGMII main port is desired, mention the same main
+ port twice.
allOf:
- if:
@@ -81,12 +89,43 @@ allOf:
- ti,dra7xx-phy-gmii-sel
- ti,dm814-phy-gmii-sel
- ti,am654-phy-gmii-sel
+ - ti,j7200-cpsw5g-phy-gmii-sel
+ - ti,j721e-cpsw9g-phy-gmii-sel
then:
properties:
'#phy-cells':
const: 1
description: CPSW port number (starting from 1)
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - ti,j7200-cpsw5g-phy-gmii-sel
+ then:
+ properties:
+ ti,qsgmii-main-ports:
+ maxItems: 1
+ items:
+ minimum: 1
+ maximum: 4
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - ti,j721e-cpsw9g-phy-gmii-sel
+ then:
+ properties:
+ ti,qsgmii-main-ports:
+ minItems: 2
+ maxItems: 2
+ items:
+ minimum: 1
+ maximum: 8
+
- if:
not:
properties:
@@ -94,6 +133,7 @@ allOf:
contains:
enum:
- ti,j7200-cpsw5g-phy-gmii-sel
+ - ti,j721e-cpsw9g-phy-gmii-sel
then:
properties:
ti,qsgmii-main-ports: false
--
2.25.1
On Wed, Sep 14, 2022 at 03:09:06PM +0530, Siddharth Vadapalli wrote:
> TI's J721e SoC supports additional PHY modes like QSGMII and SGMII
> that are not supported on earlier SoCs. Add a compatible for it.
>
> Extend ti,qsgmii-main-ports property to support selection of upto
> two main ports at once across the two QSGMII interfaces.
>
> Signed-off-by: Siddharth Vadapalli <[email protected]>
> ---
> .../bindings/phy/ti,phy-gmii-sel.yaml | 52 ++++++++++++++++---
> 1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> index da7cac537e15..1e19efab018b 100644
> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> @@ -54,6 +54,7 @@ properties:
> - ti,dm814-phy-gmii-sel
> - ti,am654-phy-gmii-sel
> - ti,j7200-cpsw5g-phy-gmii-sel
> + - ti,j721e-cpsw9g-phy-gmii-sel
>
> reg:
> maxItems: 1
> @@ -65,12 +66,19 @@ properties:
> description: |
> Required only for QSGMII mode. Array to select the port for
> QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
> - ports automatically. Any one of the 4 CPSW5G ports can act as the
> - main port with the rest of them being the QSGMII_SUB ports.
> - maxItems: 1
> - items:
> - minimum: 1
> - maximum: 4
> + ports automatically. For J7200 CPSW5G with the compatible:
> + ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
> + array of only one element, which is the port number ranging from
> + 1 to 4. For J721e CPSW9G with the compatible:
> + ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
> + of two elements, which corresponds to two potential QSGMII main
> + ports. The first element and second element of the array can both
> + range from 1 to 8 each, corresponding to two QSGMII main ports.
> + For J721e CPSW9G, to configure port 2 as the first QSGMII main
> + port and port 7 as the second QSGMII main port, we specify:
> + ti,qsgmii-main-ports = <2>, <7>;
> + If only one QSGMII main port is desired, mention the same main
> + port twice.
Two different forms for the same property name is not great. Just make a
new property if you need something different.
Rob
Hello Rob,
On 14/09/22 21:45, Rob Herring wrote:
> On Wed, Sep 14, 2022 at 03:09:06PM +0530, Siddharth Vadapalli wrote:
>> TI's J721e SoC supports additional PHY modes like QSGMII and SGMII
>> that are not supported on earlier SoCs. Add a compatible for it.
>>
>> Extend ti,qsgmii-main-ports property to support selection of upto
>> two main ports at once across the two QSGMII interfaces.
>>
>> Signed-off-by: Siddharth Vadapalli <[email protected]>
>> ---
>> .../bindings/phy/ti,phy-gmii-sel.yaml | 52 ++++++++++++++++---
>> 1 file changed, 46 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> index da7cac537e15..1e19efab018b 100644
>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> @@ -54,6 +54,7 @@ properties:
>> - ti,dm814-phy-gmii-sel
>> - ti,am654-phy-gmii-sel
>> - ti,j7200-cpsw5g-phy-gmii-sel
>> + - ti,j721e-cpsw9g-phy-gmii-sel
>>
>> reg:
>> maxItems: 1
>> @@ -65,12 +66,19 @@ properties:
>> description: |
>> Required only for QSGMII mode. Array to select the port for
>> QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>> - ports automatically. Any one of the 4 CPSW5G ports can act as the
>> - main port with the rest of them being the QSGMII_SUB ports.
>> - maxItems: 1
>> - items:
>> - minimum: 1
>> - maximum: 4
>> + ports automatically. For J7200 CPSW5G with the compatible:
>> + ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
>> + array of only one element, which is the port number ranging from
>> + 1 to 4. For J721e CPSW9G with the compatible:
>> + ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
>> + of two elements, which corresponds to two potential QSGMII main
>> + ports. The first element and second element of the array can both
>> + range from 1 to 8 each, corresponding to two QSGMII main ports.
>> + For J721e CPSW9G, to configure port 2 as the first QSGMII main
>> + port and port 7 as the second QSGMII main port, we specify:
>> + ti,qsgmii-main-ports = <2>, <7>;
>> + If only one QSGMII main port is desired, mention the same main
>> + port twice.
>
> Two different forms for the same property name is not great. Just make a
> new property if you need something different.
Thank you for reviewing the patch. Based on the discussion for the
previous series at [1], I had planned to reuse the same property
"ti,qsgmii-main-ports" for TI's J721e device too. The reason for this is
that the property represents the same feature on both devices which is
that of the QSGMII main port. The only difference between the two of
them is that J7200's CPSW5G has 4 external ports while J721e's CPSW9G
has 8 external ports. Thus, J7200 can have at most one QSGMII main port
while J721e can have up to two. Adding a new property which describes
the same feature appears to be redundant to me. Please let me know.
[1]-> https://lore.kernel.org/r/[email protected]/
Regards,
Siddharth.
On 15/09/2022 07:28, Siddharth Vadapalli wrote:
>>> @@ -65,12 +66,19 @@ properties:
>>> description: |
>>> Required only for QSGMII mode. Array to select the port for
>>> QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>>> - ports automatically. Any one of the 4 CPSW5G ports can act as the
>>> - main port with the rest of them being the QSGMII_SUB ports.
>>> - maxItems: 1
>>> - items:
>>> - minimum: 1
>>> - maximum: 4
>>> + ports automatically. For J7200 CPSW5G with the compatible:
>>> + ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
>>> + array of only one element, which is the port number ranging from
>>> + 1 to 4. For J721e CPSW9G with the compatible:
>>> + ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
>>> + of two elements, which corresponds to two potential QSGMII main
>>> + ports. The first element and second element of the array can both
>>> + range from 1 to 8 each, corresponding to two QSGMII main ports.
>>> + For J721e CPSW9G, to configure port 2 as the first QSGMII main
>>> + port and port 7 as the second QSGMII main port, we specify:
>>> + ti,qsgmii-main-ports = <2>, <7>;
>>> + If only one QSGMII main port is desired, mention the same main
>>> + port twice.
>>
>> Two different forms for the same property name is not great. Just make a
>> new property if you need something different.
>
> Thank you for reviewing the patch. Based on the discussion for the
> previous series at [1], I had planned to reuse the same property
> "ti,qsgmii-main-ports" for TI's J721e device too. The reason for this is
> that the property represents the same feature on both devices which is
> that of the QSGMII main port. The only difference between the two of
> them is that J7200's CPSW5G has 4 external ports while J721e's CPSW9G
> has 8 external ports. Thus, J7200 can have at most one QSGMII main port
> while J721e can have up to two. Adding a new property which describes
> the same feature appears to be redundant to me. Please let me know.
>
The trouble is that you wrote the description like it were two different
properties (for xx this is one element, for yy this is something else).
You need to describe the property in unified way.
Best regards,
Krzysztof
On 14/09/2022 11:39, Siddharth Vadapalli wrote:
> TI's J721e SoC supports additional PHY modes like QSGMII and SGMII
> that are not supported on earlier SoCs. Add a compatible for it.
>
> Extend ti,qsgmii-main-ports property to support selection of upto
> two main ports at once across the two QSGMII interfaces.
>
> Signed-off-by: Siddharth Vadapalli <[email protected]>
> ---
> .../bindings/phy/ti,phy-gmii-sel.yaml | 52 ++++++++++++++++---
> 1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> index da7cac537e15..1e19efab018b 100644
> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> @@ -54,6 +54,7 @@ properties:
> - ti,dm814-phy-gmii-sel
> - ti,am654-phy-gmii-sel
> - ti,j7200-cpsw5g-phy-gmii-sel
> + - ti,j721e-cpsw9g-phy-gmii-sel
>
> reg:
> maxItems: 1
> @@ -65,12 +66,19 @@ properties:
> description: |
> Required only for QSGMII mode. Array to select the port for
> QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
> - ports automatically. Any one of the 4 CPSW5G ports can act as the
> - main port with the rest of them being the QSGMII_SUB ports.
> - maxItems: 1
> - items:
> - minimum: 1
> - maximum: 4
minItems: 1
maxItems: 2
items:
minimum: 1
maximum: 8
> + ports automatically. For J7200 CPSW5G with the compatible:
> + ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
> + array of only one element, which is the port number ranging from
> + 1 to 4. For J721e CPSW9G with the compatible:
> + ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
> + of two elements, which corresponds to two potential QSGMII main
> + ports. The first element and second element of the array can both
> + range from 1 to 8 each, corresponding to two QSGMII main ports.
> + For J721e CPSW9G, to configure port 2 as the first QSGMII main
> + port and port 7 as the second QSGMII main port, we specify:
> + ti,qsgmii-main-ports = <2>, <7>;
> + If only one QSGMII main port is desired, mention the same main
> + port twice.
>
> allOf:
> - if:
> @@ -81,12 +89,43 @@ allOf:
> - ti,dra7xx-phy-gmii-sel
> - ti,dm814-phy-gmii-sel
> - ti,am654-phy-gmii-sel
> + - ti,j7200-cpsw5g-phy-gmii-sel
> + - ti,j721e-cpsw9g-phy-gmii-sel
> then:
> properties:
> '#phy-cells':
> const: 1
> description: CPSW port number (starting from 1)
>
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - ti,j7200-cpsw5g-phy-gmii-sel
> + then:
> + properties:
> + ti,qsgmii-main-ports:
> + maxItems: 1
> + items:
> + minimum: 1
> + maximum: 4
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - ti,j721e-cpsw9g-phy-gmii-sel
> + then:
> + properties:
> + ti,qsgmii-main-ports:
> + minItems: 2
> + maxItems: 2
> + items:
> + minimum: 1
> + maximum: 8
> +
> - if:
> not:
> properties:
> @@ -94,6 +133,7 @@ allOf:
> contains:
> enum:
> - ti,j7200-cpsw5g-phy-gmii-sel
> + - ti,j721e-cpsw9g-phy-gmii-sel
> then:
> properties:
> ti,qsgmii-main-ports: false
This is interesting here... Did you test the bindings with your DTS?
Best regards,
Krzysztof
Hello Krzysztof,
On 19/09/22 15:45, Krzysztof Kozlowski wrote:
> On 14/09/2022 11:39, Siddharth Vadapalli wrote:
>> TI's J721e SoC supports additional PHY modes like QSGMII and SGMII
>> that are not supported on earlier SoCs. Add a compatible for it.
>>
>> Extend ti,qsgmii-main-ports property to support selection of upto
>> two main ports at once across the two QSGMII interfaces.
>>
>> Signed-off-by: Siddharth Vadapalli <[email protected]>
>> ---
>> .../bindings/phy/ti,phy-gmii-sel.yaml | 52 ++++++++++++++++---
>> 1 file changed, 46 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> index da7cac537e15..1e19efab018b 100644
>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> @@ -54,6 +54,7 @@ properties:
>> - ti,dm814-phy-gmii-sel
>> - ti,am654-phy-gmii-sel
>> - ti,j7200-cpsw5g-phy-gmii-sel
>> + - ti,j721e-cpsw9g-phy-gmii-sel
>>
>> reg:
>> maxItems: 1
>> @@ -65,12 +66,19 @@ properties:
>> description: |
>> Required only for QSGMII mode. Array to select the port for
>> QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>> - ports automatically. Any one of the 4 CPSW5G ports can act as the
>> - main port with the rest of them being the QSGMII_SUB ports.
>> - maxItems: 1
>> - items:
>> - minimum: 1
>> - maximum: 4
>
> minItems: 1
> maxItems: 2
> items:
> minimum: 1
> maximum: 8
Thank you for reviewing the patch. I assume that you want me to mention
the values for "minItems", "maxItems", "minimum" and "maximum" here and
modify them later based on the compatible where applicable. I will do so
in the v2 series.
>
>> + ports automatically. For J7200 CPSW5G with the compatible:
>> + ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
>> + array of only one element, which is the port number ranging from
>> + 1 to 4. For J721e CPSW9G with the compatible:
>> + ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
>> + of two elements, which corresponds to two potential QSGMII main
>> + ports. The first element and second element of the array can both
>> + range from 1 to 8 each, corresponding to two QSGMII main ports.
>> + For J721e CPSW9G, to configure port 2 as the first QSGMII main
>> + port and port 7 as the second QSGMII main port, we specify:
>> + ti,qsgmii-main-ports = <2>, <7>;
>> + If only one QSGMII main port is desired, mention the same main
>> + port twice.
>>
>> allOf:
>> - if:
>> @@ -81,12 +89,43 @@ allOf:
>> - ti,dra7xx-phy-gmii-sel
>> - ti,dm814-phy-gmii-sel
>> - ti,am654-phy-gmii-sel
>> + - ti,j7200-cpsw5g-phy-gmii-sel
>> + - ti,j721e-cpsw9g-phy-gmii-sel
>> then:
>> properties:
>> '#phy-cells':
>> const: 1
>> description: CPSW port number (starting from 1)
>>
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - ti,j7200-cpsw5g-phy-gmii-sel
>> + then:
>> + properties:
>> + ti,qsgmii-main-ports:
>> + maxItems: 1
>> + items:
>> + minimum: 1
>> + maximum: 4
>> +
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - ti,j721e-cpsw9g-phy-gmii-sel
>> + then:
>> + properties:
>> + ti,qsgmii-main-ports:
>> + minItems: 2
>> + maxItems: 2
>> + items:
>> + minimum: 1
>> + maximum: 8
>> +
>> - if:
>> not:
>> properties:
>> @@ -94,6 +133,7 @@ allOf:
>> contains:
>> enum:
>> - ti,j7200-cpsw5g-phy-gmii-sel
>> + - ti,j721e-cpsw9g-phy-gmii-sel
>> then:
>> properties:
>> ti,qsgmii-main-ports: false
>
> This is interesting here... Did you test the bindings with your DTS?
Yes, I tried it out with different compatibles in the DTS file for the
node, making sure that the property "ti,qsgmii-main-ports" is allowed
only for the "ti,j7200-cpsw5g-phy-gmii-sel" and the
"ti,j721e-cpsw9g-phy-gmii-sel" compatibles. Additionally, I also tested
that the "minItems", "maxItems", "minimum" and "maximum" checks apply.
All of the rules within the "allOf", are enforced one after the other in
sequence, based on my testing. Please let me know in case of any
suggestions to implement it in a better way.
Regards,
Siddharth.
Hello Krzysztof,
On 19/09/22 15:47, Krzysztof Kozlowski wrote:
> On 15/09/2022 07:28, Siddharth Vadapalli wrote:
>>>> @@ -65,12 +66,19 @@ properties:
>>>> description: |
>>>> Required only for QSGMII mode. Array to select the port for
>>>> QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>>>> - ports automatically. Any one of the 4 CPSW5G ports can act as the
>>>> - main port with the rest of them being the QSGMII_SUB ports.
>>>> - maxItems: 1
>>>> - items:
>>>> - minimum: 1
>>>> - maximum: 4
>>>> + ports automatically. For J7200 CPSW5G with the compatible:
>>>> + ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
>>>> + array of only one element, which is the port number ranging from
>>>> + 1 to 4. For J721e CPSW9G with the compatible:
>>>> + ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
>>>> + of two elements, which corresponds to two potential QSGMII main
>>>> + ports. The first element and second element of the array can both
>>>> + range from 1 to 8 each, corresponding to two QSGMII main ports.
>>>> + For J721e CPSW9G, to configure port 2 as the first QSGMII main
>>>> + port and port 7 as the second QSGMII main port, we specify:
>>>> + ti,qsgmii-main-ports = <2>, <7>;
>>>> + If only one QSGMII main port is desired, mention the same main
>>>> + port twice.
>>>
>>> Two different forms for the same property name is not great. Just make a
>>> new property if you need something different.
>>
>> Thank you for reviewing the patch. Based on the discussion for the
>> previous series at [1], I had planned to reuse the same property
>> "ti,qsgmii-main-ports" for TI's J721e device too. The reason for this is
>> that the property represents the same feature on both devices which is
>> that of the QSGMII main port. The only difference between the two of
>> them is that J7200's CPSW5G has 4 external ports while J721e's CPSW9G
>> has 8 external ports. Thus, J7200 can have at most one QSGMII main port
>> while J721e can have up to two. Adding a new property which describes
>> the same feature appears to be redundant to me. Please let me know.
>>
>
> The trouble is that you wrote the description like it were two different
> properties (for xx this is one element, for yy this is something else).
> You need to describe the property in unified way.
Thank you for reviewing the patch. I plan to update the description to
the following:
"Required only for QSGMII mode. Array to select the port/s for QSGMII
main mode. The size of the array corresponds to the number of QSGMII
interfaces and thus, the number of distinct QSGMII main ports, supported
by the device. If the device supports two QSGMII interfaces but only one
QSGMII interface is desired, repeat the QSGMII main port value
corresponding to the QSGMII interface in the array."
I intend to describe the property in detail to help users understand the
property and its usage better. In the process, I might have
unintentionally made it appear as two different properties in the
previous description. I hope the new description shows that the property
describes the same feature across devices while making its usage clear
to the users at the same time. Please let me know if this is fine.
Regards,
Siddharth.
On 20/09/2022 06:56, Siddharth Vadapalli wrote:
>>> Thank you for reviewing the patch. Based on the discussion for the
>>> previous series at [1], I had planned to reuse the same property
>>> "ti,qsgmii-main-ports" for TI's J721e device too. The reason for this is
>>> that the property represents the same feature on both devices which is
>>> that of the QSGMII main port. The only difference between the two of
>>> them is that J7200's CPSW5G has 4 external ports while J721e's CPSW9G
>>> has 8 external ports. Thus, J7200 can have at most one QSGMII main port
>>> while J721e can have up to two. Adding a new property which describes
>>> the same feature appears to be redundant to me. Please let me know.
>>>
>>
>> The trouble is that you wrote the description like it were two different
>> properties (for xx this is one element, for yy this is something else).
>> You need to describe the property in unified way.
>
> Thank you for reviewing the patch. I plan to update the description to
> the following:
> "Required only for QSGMII mode. Array to select the port/s for QSGMII
> main mode. The size of the array corresponds to the number of QSGMII
> interfaces and thus, the number of distinct QSGMII main ports, supported
> by the device. If the device supports two QSGMII interfaces but only one
> QSGMII interface is desired, repeat the QSGMII main port value
> corresponding to the QSGMII interface in the array."
>
> I intend to describe the property in detail to help users understand the
> property and its usage better. In the process, I might have
> unintentionally made it appear as two different properties in the
> previous description. I hope the new description shows that the property
> describes the same feature across devices while making its usage clear
> to the users at the same time. Please let me know if this is fine.
Sounds good to me.
Best regards,
Krzysztof
On 20/09/2022 06:27, Siddharth Vadapalli wrote:
> Hello Krzysztof,
>
>>> + then:
>>> + properties:
>>> + ti,qsgmii-main-ports:
>>> + minItems: 2
>>> + maxItems: 2
>>> + items:
>>> + minimum: 1
>>> + maximum: 8
>>> +
>>> - if:
>>> not:
>>> properties:
>>> @@ -94,6 +133,7 @@ allOf:
>>> contains:
>>> enum:
>>> - ti,j7200-cpsw5g-phy-gmii-sel
>>> + - ti,j721e-cpsw9g-phy-gmii-sel
>>> then:
>>> properties:
>>> ti,qsgmii-main-ports: false
>>
>> This is interesting here... Did you test the bindings with your DTS?
>
> Yes, I tried it out with different compatibles in the DTS file for the
> node, making sure that the property "ti,qsgmii-main-ports" is allowed
> only for the "ti,j7200-cpsw5g-phy-gmii-sel" and the
> "ti,j721e-cpsw9g-phy-gmii-sel" compatibles. Additionally, I also tested
> that the "minItems", "maxItems", "minimum" and "maximum" checks apply.
> All of the rules within the "allOf", are enforced one after the other in
> sequence, based on my testing. Please let me know in case of any
> suggestions to implement it in a better way.
Great! I think I see now what I missed previously. The last hunk with
"ti,qsgmii-main-ports: false" is in a if: with negation ("not:")?
Best regards,
Krzysztof
Hello Krzysztof,
On 21/09/22 12:09, Krzysztof Kozlowski wrote:
> On 20/09/2022 06:27, Siddharth Vadapalli wrote:
>> Hello Krzysztof,
>>
>>>> + then:
>>>> + properties:
>>>> + ti,qsgmii-main-ports:
>>>> + minItems: 2
>>>> + maxItems: 2
>>>> + items:
>>>> + minimum: 1
>>>> + maximum: 8
>>>> +
>>>> - if:
>>>> not:
>>>> properties:
>>>> @@ -94,6 +133,7 @@ allOf:
>>>> contains:
>>>> enum:
>>>> - ti,j7200-cpsw5g-phy-gmii-sel
>>>> + - ti,j721e-cpsw9g-phy-gmii-sel
>>>> then:
>>>> properties:
>>>> ti,qsgmii-main-ports: false
>>>
>>> This is interesting here... Did you test the bindings with your DTS?
>>
>> Yes, I tried it out with different compatibles in the DTS file for the
>> node, making sure that the property "ti,qsgmii-main-ports" is allowed
>> only for the "ti,j7200-cpsw5g-phy-gmii-sel" and the
>> "ti,j721e-cpsw9g-phy-gmii-sel" compatibles. Additionally, I also tested
>> that the "minItems", "maxItems", "minimum" and "maximum" checks apply.
>> All of the rules within the "allOf", are enforced one after the other in
>> sequence, based on my testing. Please let me know in case of any
>> suggestions to implement it in a better way.
>
> Great! I think I see now what I missed previously. The last hunk with
> "ti,qsgmii-main-ports: false" is in a if: with negation ("not:")?
Yes, the newly added compatible "ti,j721e-cpsw9g-phy-gmii-sel" is placed
within an "if:" followed by a "not:", along with the already existing
"ti,j7200-cpsw5g-phy-gmii-sel" compatible. With this,
"ti,qsgmii-main-ports" is allowed only for the aforementioned
compatibles and disallowed for the rest.
Regards,
Siddharth.