[PATCH v3 0/3] Add support to PHY GMII SEL for J721e CPSW9G QSGMII

[PATCH v3 0/3] Add support to PHY GMII SEL for J721e CPSW9G QSGMII

[Siddharth Vadapalli]

Add compatible for J721e CPSW9G, which contains 8 external ports and 1
internal host port.

Update existing approach of using compatible to differentiate between
devices that support QSGMII mode and those that don't. The new
approach involves storing the number of qsgmii main ports for the device
in the num_qsgmii_main_ports member of the "struct phy_gmii_sel_soc_data".
This approach makes it scalable for newer devices.

=========
Changelog
=========
v2 -> v3:
1. Run 'make DT_CHECKER_FLAGS=-m dt_binding_check' and fix errors and
   warnings corresponding to the patch for:
   Documentation/devicetree/bindings/phy/ti,phy-gmii-sel
   with the latest dt-schema and yamllint.

v1 -> v2:
1. Drop all patches corresponding to SGMII mode. This is done since I do
   not have a method to test SGMII in the standard mode which uses an
   SGMII PHY. The previous series used SGMII in a fixed-link mode,
   bypassing the SGMII PHY. I will post the SGMII patches in a future
   series after testing them.
2. Update description for the property "ti,qsgmii-main-ports", to describe
   it in a unified way across the compatibles.
3. Add minItems, maxItems, and items at the top, where the property
   "ti,qsgmii-main-ports" is first defined. Modify them later
   appropriately, based on the compatible.
4. Update the method to fetch the property "ti,qsgmii-main-ports" from the
   device-tree, to make it scalable.
5. Use dev_err() when the value(s) provided in the device-tree for the
   property "ti,qsgmii-main-ports" is/are invalid.

v2:
https://lore.kernel.org/r/20221018084333.149790-1-s-vadapalli@ti.com/
v1:
https://lore.kernel.org/r/20220914093911.187764-1-s-vadapalli@ti.com/

Siddharth Vadapalli (3):
  dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e
  phy: ti: gmii-sel: Update methods for fetching and using qsgmii main
    port
  phy: ti: gmii-sel: Add support for CPSW9G GMII SEL in J721e

 .../bindings/phy/ti,phy-gmii-sel.yaml         | 48 ++++++++++++++++---
 drivers/phy/ti/phy-gmii-sel.c                 | 42 +++++++++++++---
 2 files changed, 77 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Siddharth Vadapalli]

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 <s-vadapalli@ti.com>
---
 .../bindings/phy/ti,phy-gmii-sel.yaml         | 48 ++++++++++++++++---
 1 file changed, 42 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..3a6d686383cf 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
@@ -63,14 +64,17 @@ properties:
   ti,qsgmii-main-ports:
     $ref: /schemas/types.yaml#/definitions/uint32-array
     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
+      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.
+    minItems: 1
+    maxItems: 2
     items:
       minimum: 1
-      maximum: 4
+      maximum: 8
 
 allOf:
   - if:
@@ -81,12 +85,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 +129,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

[PATCH v3 2/3] phy: ti: gmii-sel: Update methods for fetching and using qsgmii main port

[Siddharth Vadapalli]

The number of QSGMII main ports are specific to the device. TI's J7200 for
which the QSGMII main port property is fetched from the device-tree has
only one QSGMII main port. However, devices like TI's J721e support up to
two QSGMII main ports. Thus, the existing methods for fetching and using
the QSGMII main port are not scalable.

Update the existing methods for handling the QSGMII main ports and its
associated requirements to make it scalable for future devices.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/phy/ti/phy-gmii-sel.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
index 0bcfd6d96b4d..c8f30d2e1f46 100644
--- a/drivers/phy/ti/phy-gmii-sel.c
+++ b/drivers/phy/ti/phy-gmii-sel.c
@@ -50,6 +50,7 @@ struct phy_gmii_sel_soc_data {
 	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
 	bool use_of_data;
 	u64 extra_modes;
+	u32 num_qsgmii_main_ports;
 };
 
 struct phy_gmii_sel_priv {
@@ -213,6 +214,8 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
 	.use_of_data = true,
 	.regfields = phy_gmii_sel_fields_am654,
 	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
+	.num_ports = 4,
+	.num_qsgmii_main_ports = 1,
 };
 
 static const struct of_device_id phy_gmii_sel_id_table[] = {
@@ -378,11 +381,13 @@ static int phy_gmii_sel_init_ports(struct phy_gmii_sel_priv *priv)
 static int phy_gmii_sel_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	const struct phy_gmii_sel_soc_data *soc_data;
 	struct device_node *node = dev->of_node;
 	const struct of_device_id *of_id;
 	struct phy_gmii_sel_priv *priv;
 	u32 main_ports = 1;
 	int ret;
+	u32 i;
 
 	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
 	if (!of_id)
@@ -394,16 +399,26 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
 
 	priv->dev = &pdev->dev;
 	priv->soc_data = of_id->data;
+	soc_data = priv->soc_data;
 	priv->num_ports = priv->soc_data->num_ports;
-	of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports);
+	priv->qsgmii_main_ports = 0;
+
 	/*
-	 * Ensure that main_ports is within bounds. If the property
-	 * ti,qsgmii-main-ports is not mentioned, or the value mentioned
-	 * is out of bounds, default to 1.
+	 * Based on the compatible, try to read the appropriate number of
+	 * QSGMII main ports from the "ti,qsgmii-main-ports" property from
+	 * the device-tree node.
 	 */
-	if (main_ports < 1 || main_ports > 4)
-		main_ports = 1;
-	priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports);
+	for (i = 0; i < soc_data->num_qsgmii_main_ports; i++) {
+		of_property_read_u32_index(node, "ti,qsgmii-main-ports", i, &main_ports);
+		/*
+		 * Ensure that main_ports is within bounds.
+		 */
+		if (main_ports < 1 || main_ports > soc_data->num_ports) {
+			dev_err(dev, "Invalid qsgmii main port provided\n");
+			return -EINVAL;
+		}
+		priv->qsgmii_main_ports |= PHY_GMII_PORT(main_ports);
+	}
 
 	priv->regmap = syscon_node_to_regmap(node->parent);
 	if (IS_ERR(priv->regmap)) {
-- 
2.25.1

[PATCH v3 3/3] phy: ti: gmii-sel: Add support for CPSW9G GMII SEL in J721e

[Siddharth Vadapalli]

Each of the CPSW9G ports in J721e support additional modes like QSGMII.
Add a new compatible for J721e to support the additional modes.

In TI's J721e, each of the CPSW9G ethernet interfaces can act as a
QSGMII main or QSGMII-SUB port. The QSGMII main interface is responsible
for performing auto-negotiation between the MAC and the PHY while the rest
of the interfaces are designated as QSGMII-SUB interfaces, indicating that
they will not be taking part in the auto-negotiation process.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/phy/ti/phy-gmii-sel.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
index c8f30d2e1f46..8c667819c39a 100644
--- a/drivers/phy/ti/phy-gmii-sel.c
+++ b/drivers/phy/ti/phy-gmii-sel.c
@@ -218,6 +218,15 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
 	.num_qsgmii_main_ports = 1,
 };
 
+static const
+struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw9g_soc_j721e = {
+	.use_of_data = true,
+	.regfields = phy_gmii_sel_fields_am654,
+	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
+	.num_ports = 8,
+	.num_qsgmii_main_ports = 2,
+};
+
 static const struct of_device_id phy_gmii_sel_id_table[] = {
 	{
 		.compatible	= "ti,am3352-phy-gmii-sel",
@@ -243,6 +252,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = {
 		.compatible	= "ti,j7200-cpsw5g-phy-gmii-sel",
 		.data		= &phy_gmii_sel_cpsw5g_soc_j7200,
 	},
+	{
+		.compatible	= "ti,j721e-cpsw9g-phy-gmii-sel",
+		.data		= &phy_gmii_sel_cpsw9g_soc_j721e,
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table);
-- 
2.25.1

Re: [PATCH v3 1/3] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Rob Herring]

On Wed, 26 Oct 2022 13:15:30 +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 <s-vadapalli@ti.com>
> ---
>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 48 ++++++++++++++++---
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 2/3] phy: ti: gmii-sel: Update methods for fetching and using qsgmii main port

[Roger Quadros]

Hi Siddharth,

On 26/10/2022 10:45, Siddharth Vadapalli wrote:
> The number of QSGMII main ports are specific to the device. TI's J7200 for
> which the QSGMII main port property is fetched from the device-tree has
> only one QSGMII main port. However, devices like TI's J721e support up to
> two QSGMII main ports. Thus, the existing methods for fetching and using
> the QSGMII main port are not scalable.
> 
> Update the existing methods for handling the QSGMII main ports and its
> associated requirements to make it scalable for future devices.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  drivers/phy/ti/phy-gmii-sel.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
> index 0bcfd6d96b4d..c8f30d2e1f46 100644
> --- a/drivers/phy/ti/phy-gmii-sel.c
> +++ b/drivers/phy/ti/phy-gmii-sel.c
> @@ -50,6 +50,7 @@ struct phy_gmii_sel_soc_data {
>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>  	bool use_of_data;
>  	u64 extra_modes;
> +	u32 num_qsgmii_main_ports;
>  };
>  
>  struct phy_gmii_sel_priv {
> @@ -213,6 +214,8 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
>  	.use_of_data = true,
>  	.regfields = phy_gmii_sel_fields_am654,
>  	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
> +	.num_ports = 4,
> +	.num_qsgmii_main_ports = 1,
>  };
>  
>  static const struct of_device_id phy_gmii_sel_id_table[] = {
> @@ -378,11 +381,13 @@ static int phy_gmii_sel_init_ports(struct phy_gmii_sel_priv *priv)
>  static int phy_gmii_sel_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	const struct phy_gmii_sel_soc_data *soc_data;
>  	struct device_node *node = dev->of_node;
>  	const struct of_device_id *of_id;
>  	struct phy_gmii_sel_priv *priv;
>  	u32 main_ports = 1;
>  	int ret;
> +	u32 i;
>  
>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
>  	if (!of_id)
> @@ -394,16 +399,26 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>  
>  	priv->dev = &pdev->dev;
>  	priv->soc_data = of_id->data;
> +	soc_data = priv->soc_data;
>  	priv->num_ports = priv->soc_data->num_ports;
> -	of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports);
> +	priv->qsgmii_main_ports = 0;
> +
>  	/*
> -	 * Ensure that main_ports is within bounds. If the property
> -	 * ti,qsgmii-main-ports is not mentioned, or the value mentioned
> -	 * is out of bounds, default to 1.
> +	 * Based on the compatible, try to read the appropriate number of
> +	 * QSGMII main ports from the "ti,qsgmii-main-ports" property from
> +	 * the device-tree node.
>  	 */
> -	if (main_ports < 1 || main_ports > 4)
> -		main_ports = 1;
> -	priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports);
> +	for (i = 0; i < soc_data->num_qsgmii_main_ports; i++) {
> +		of_property_read_u32_index(node, "ti,qsgmii-main-ports", i, &main_ports);
> +		/*
> +		 * Ensure that main_ports is within bounds.
> +		 */
> +		if (main_ports < 1 || main_ports > soc_data->num_ports) {
> +			dev_err(dev, "Invalid qsgmii main port provided\n");

nit: This message is a bit misleading if the property does not exist in DT.

How about just "Invalid ti,qsgmii-main-ports"

> +			return -EINVAL;
> +		}
> +		priv->qsgmii_main_ports |= PHY_GMII_PORT(main_ports);
> +	}
>  
>  	priv->regmap = syscon_node_to_regmap(node->parent);
>  	if (IS_ERR(priv->regmap)) {

Reviewed-by: Roger Quadros <rogerq@kernel.org>

cheers,
-roger

Re: [PATCH v3 3/3] phy: ti: gmii-sel: Add support for CPSW9G GMII SEL in J721e

[Roger Quadros]

On 26/10/2022 10:45, Siddharth Vadapalli wrote:
> Each of the CPSW9G ports in J721e support additional modes like QSGMII.
> Add a new compatible for J721e to support the additional modes.
> 
> In TI's J721e, each of the CPSW9G ethernet interfaces can act as a
> QSGMII main or QSGMII-SUB port. The QSGMII main interface is responsible
> for performing auto-negotiation between the MAC and the PHY while the rest
> of the interfaces are designated as QSGMII-SUB interfaces, indicating that
> they will not be taking part in the auto-negotiation process.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

> ---
>  drivers/phy/ti/phy-gmii-sel.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
> index c8f30d2e1f46..8c667819c39a 100644
> --- a/drivers/phy/ti/phy-gmii-sel.c
> +++ b/drivers/phy/ti/phy-gmii-sel.c
> @@ -218,6 +218,15 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
>  	.num_qsgmii_main_ports = 1,
>  };
>  
> +static const
> +struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw9g_soc_j721e = {
> +	.use_of_data = true,
> +	.regfields = phy_gmii_sel_fields_am654,
> +	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
> +	.num_ports = 8,
> +	.num_qsgmii_main_ports = 2,
> +};
> +
>  static const struct of_device_id phy_gmii_sel_id_table[] = {
>  	{
>  		.compatible	= "ti,am3352-phy-gmii-sel",
> @@ -243,6 +252,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = {
>  		.compatible	= "ti,j7200-cpsw5g-phy-gmii-sel",
>  		.data		= &phy_gmii_sel_cpsw5g_soc_j7200,
>  	},
> +	{
> +		.compatible	= "ti,j721e-cpsw9g-phy-gmii-sel",
> +		.data		= &phy_gmii_sel_cpsw9g_soc_j721e,
> +	},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table);

cheers,
-roger

Re: [PATCH v3 2/3] phy: ti: gmii-sel: Update methods for fetching and using qsgmii main port

[Siddharth Vadapalli]

Hello Roger,

On 28/10/22 15:53, Roger Quadros wrote:
> Hi Siddharth,
> 
> On 26/10/2022 10:45, Siddharth Vadapalli wrote:
>> The number of QSGMII main ports are specific to the device. TI's J7200 for
>> which the QSGMII main port property is fetched from the device-tree has
>> only one QSGMII main port. However, devices like TI's J721e support up to
>> two QSGMII main ports. Thus, the existing methods for fetching and using
>> the QSGMII main port are not scalable.
>>
>> Update the existing methods for handling the QSGMII main ports and its
>> associated requirements to make it scalable for future devices.
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  drivers/phy/ti/phy-gmii-sel.c | 29 ++++++++++++++++++++++-------
>>  1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
>> index 0bcfd6d96b4d..c8f30d2e1f46 100644
>> --- a/drivers/phy/ti/phy-gmii-sel.c
>> +++ b/drivers/phy/ti/phy-gmii-sel.c
>> @@ -50,6 +50,7 @@ struct phy_gmii_sel_soc_data {
>>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>>  	bool use_of_data;
>>  	u64 extra_modes;
>> +	u32 num_qsgmii_main_ports;
>>  };
>>  
>>  struct phy_gmii_sel_priv {
>> @@ -213,6 +214,8 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
>>  	.use_of_data = true,
>>  	.regfields = phy_gmii_sel_fields_am654,
>>  	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
>> +	.num_ports = 4,
>> +	.num_qsgmii_main_ports = 1,
>>  };
>>  
>>  static const struct of_device_id phy_gmii_sel_id_table[] = {
>> @@ -378,11 +381,13 @@ static int phy_gmii_sel_init_ports(struct phy_gmii_sel_priv *priv)
>>  static int phy_gmii_sel_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> +	const struct phy_gmii_sel_soc_data *soc_data;
>>  	struct device_node *node = dev->of_node;
>>  	const struct of_device_id *of_id;
>>  	struct phy_gmii_sel_priv *priv;
>>  	u32 main_ports = 1;
>>  	int ret;
>> +	u32 i;
>>  
>>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
>>  	if (!of_id)
>> @@ -394,16 +399,26 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>  
>>  	priv->dev = &pdev->dev;
>>  	priv->soc_data = of_id->data;
>> +	soc_data = priv->soc_data;
>>  	priv->num_ports = priv->soc_data->num_ports;
>> -	of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports);
>> +	priv->qsgmii_main_ports = 0;
>> +
>>  	/*
>> -	 * Ensure that main_ports is within bounds. If the property
>> -	 * ti,qsgmii-main-ports is not mentioned, or the value mentioned
>> -	 * is out of bounds, default to 1.
>> +	 * Based on the compatible, try to read the appropriate number of
>> +	 * QSGMII main ports from the "ti,qsgmii-main-ports" property from
>> +	 * the device-tree node.
>>  	 */
>> -	if (main_ports < 1 || main_ports > 4)
>> -		main_ports = 1;
>> -	priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports);
>> +	for (i = 0; i < soc_data->num_qsgmii_main_ports; i++) {
>> +		of_property_read_u32_index(node, "ti,qsgmii-main-ports", i, &main_ports);
>> +		/*
>> +		 * Ensure that main_ports is within bounds.
>> +		 */
>> +		if (main_ports < 1 || main_ports > soc_data->num_ports) {
>> +			dev_err(dev, "Invalid qsgmii main port provided\n");
> 
> nit: This message is a bit misleading if the property does not exist in DT.
> 
> How about just "Invalid ti,qsgmii-main-ports"

Thank you for reviewing the patch. The variable "main_ports" has been
initialized to 1 at the top. Thus, the only way the error condition is
entered is if "ti,qsgmii-main-ports" is mentioned in the device-tree
with an invalid value. If "ti,qsgmii-main-ports" is not mentioned in the
device-tree, then "main_ports" continues being 1, since the function
"of_property_read_u32_index()" does not modify "main_ports" if
"ti,qsgmii-main-ports" is not present in the device-tree. Thus, in this
case, the error condition isn't reached.

Regards,
Siddharth.

Re: [PATCH v3 2/3] phy: ti: gmii-sel: Update methods for fetching and using qsgmii main port

[Roger Quadros]

On 28/10/2022 13:32, Siddharth Vadapalli wrote:
> Hello Roger,
> 
> On 28/10/22 15:53, Roger Quadros wrote:
>> Hi Siddharth,
>>
>> On 26/10/2022 10:45, Siddharth Vadapalli wrote:
>>> The number of QSGMII main ports are specific to the device. TI's J7200 for
>>> which the QSGMII main port property is fetched from the device-tree has
>>> only one QSGMII main port. However, devices like TI's J721e support up to
>>> two QSGMII main ports. Thus, the existing methods for fetching and using
>>> the QSGMII main port are not scalable.
>>>
>>> Update the existing methods for handling the QSGMII main ports and its
>>> associated requirements to make it scalable for future devices.
>>>
>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>> ---
>>>  drivers/phy/ti/phy-gmii-sel.c | 29 ++++++++++++++++++++++-------
>>>  1 file changed, 22 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
>>> index 0bcfd6d96b4d..c8f30d2e1f46 100644
>>> --- a/drivers/phy/ti/phy-gmii-sel.c
>>> +++ b/drivers/phy/ti/phy-gmii-sel.c
>>> @@ -50,6 +50,7 @@ struct phy_gmii_sel_soc_data {
>>>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>>>  	bool use_of_data;
>>>  	u64 extra_modes;
>>> +	u32 num_qsgmii_main_ports;
>>>  };
>>>  
>>>  struct phy_gmii_sel_priv {
>>> @@ -213,6 +214,8 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
>>>  	.use_of_data = true,
>>>  	.regfields = phy_gmii_sel_fields_am654,
>>>  	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
>>> +	.num_ports = 4,
>>> +	.num_qsgmii_main_ports = 1,
>>>  };
>>>  
>>>  static const struct of_device_id phy_gmii_sel_id_table[] = {
>>> @@ -378,11 +381,13 @@ static int phy_gmii_sel_init_ports(struct phy_gmii_sel_priv *priv)
>>>  static int phy_gmii_sel_probe(struct platform_device *pdev)
>>>  {
>>>  	struct device *dev = &pdev->dev;
>>> +	const struct phy_gmii_sel_soc_data *soc_data;
>>>  	struct device_node *node = dev->of_node;
>>>  	const struct of_device_id *of_id;
>>>  	struct phy_gmii_sel_priv *priv;
>>>  	u32 main_ports = 1;
>>>  	int ret;
>>> +	u32 i;
>>>  
>>>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
>>>  	if (!of_id)
>>> @@ -394,16 +399,26 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>>  
>>>  	priv->dev = &pdev->dev;
>>>  	priv->soc_data = of_id->data;
>>> +	soc_data = priv->soc_data;
>>>  	priv->num_ports = priv->soc_data->num_ports;
>>> -	of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports);
>>> +	priv->qsgmii_main_ports = 0;
>>> +
>>>  	/*
>>> -	 * Ensure that main_ports is within bounds. If the property
>>> -	 * ti,qsgmii-main-ports is not mentioned, or the value mentioned
>>> -	 * is out of bounds, default to 1.
>>> +	 * Based on the compatible, try to read the appropriate number of
>>> +	 * QSGMII main ports from the "ti,qsgmii-main-ports" property from
>>> +	 * the device-tree node.
>>>  	 */
>>> -	if (main_ports < 1 || main_ports > 4)
>>> -		main_ports = 1;
>>> -	priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports);
>>> +	for (i = 0; i < soc_data->num_qsgmii_main_ports; i++) {
>>> +		of_property_read_u32_index(node, "ti,qsgmii-main-ports", i, &main_ports);
>>> +		/*
>>> +		 * Ensure that main_ports is within bounds.
>>> +		 */
>>> +		if (main_ports < 1 || main_ports > soc_data->num_ports) {
>>> +			dev_err(dev, "Invalid qsgmii main port provided\n");
>>
>> nit: This message is a bit misleading if the property does not exist in DT.
>>
>> How about just "Invalid ti,qsgmii-main-ports"
> 
> Thank you for reviewing the patch. The variable "main_ports" has been
> initialized to 1 at the top. Thus, the only way the error condition is
> entered is if "ti,qsgmii-main-ports" is mentioned in the device-tree
> with an invalid value. If "ti,qsgmii-main-ports" is not mentioned in the
> device-tree, then "main_ports" continues being 1, since the function
> "of_property_read_u32_index()" does not modify "main_ports" if
> "ti,qsgmii-main-ports" is not present in the device-tree. Thus, in this
> case, the error condition isn't reached.

You are right. No need to change the message in that case.

cheers,
-roger

Re: [PATCH v3 0/3] Add support to PHY GMII SEL for J721e CPSW9G QSGMII

[Vinod Koul]

On 26-10-22, 13:15, Siddharth Vadapalli wrote:
> Add compatible for J721e CPSW9G, which contains 8 external ports and 1
> internal host port.
> 
> Update existing approach of using compatible to differentiate between
> devices that support QSGMII mode and those that don't. The new
> approach involves storing the number of qsgmii main ports for the device
> in the num_qsgmii_main_ports member of the "struct phy_gmii_sel_soc_data".
> This approach makes it scalable for newer devices.

Applied, thanks

-- 
~Vinod