[PATCH v3 0/2] Add support for QSGMII mode

[PATCH v3 0/2] Add support for QSGMII mode

[Siddharth Vadapalli]

Add compatible for J7200 CPSW5G.

Add support for QSGMII mode in phy-gmii-sel driver for CPSW5G in J7200.

Change log:

v2 -> v3:
1. Add $ref to "phy@[0-9a-f]+$" pattern property in
   Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml.
2. Restrict the optional ti,qsgmii-main-ports property to
   ti,j7200-cpsw5g-phy-gmii-sel property by adding an else statement and
   disallowing it for other compatibles.
3. Move the "items" constraint for the ti,qsgmii-main-ports property to
   the place the property is defined.

v1->v2:
1. Rename ti,enet-ctrl-qsgmii as ti,qsgmii-main-ports.
2. Change ti,qsgmii-main-ports property from bitmask to integer.
3.Update commit message with property name as ti,qsgmii-main-ports.

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

Siddharth Vadapalli (2):
  dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200
  phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200

 .../mfd/ti,j721e-system-controller.yaml       |  6 +++
 .../bindings/phy/ti,phy-gmii-sel.yaml         | 30 +++++++++++++-
 drivers/phy/ti/phy-gmii-sel.c                 | 40 +++++++++++++++++--
 3 files changed, 72 insertions(+), 4 deletions(-)

--
2.25.1

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

[Siddharth Vadapalli]

TI's J7200 SoC supports additional PHY modes like QSGMII and SGMII
that are not supported on earlier SoCs. Add a compatible for it.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 .../mfd/ti,j721e-system-controller.yaml       |  6 ++++
 .../bindings/phy/ti,phy-gmii-sel.yaml         | 30 ++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
index 73cffc45e056..466724cb4157 100644
--- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
+++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
@@ -54,6 +54,12 @@ patternProperties:
     description:
       Clock provider for TI EHRPWM nodes.
 
+  "phy@[0-9a-f]+$":
+    type: object
+    $ref: ../phy/ti,phy-gmii-sel.yaml
+    description:
+      This is the register to set phy mode through phy-gmii-sel driver.
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
index ff8a6d9eb153..0ffb97f1a77c 100644
--- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
@@ -53,12 +53,24 @@ properties:
       - ti,am43xx-phy-gmii-sel
       - ti,dm814-phy-gmii-sel
       - ti,am654-phy-gmii-sel
+      - ti,j7200-cpsw5g-phy-gmii-sel
 
   reg:
     maxItems: 1
 
   '#phy-cells': true
 
+  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.
+    items:
+      minimum: 1
+      maximum: 4
+
 allOf:
   - if:
       properties:
@@ -73,6 +85,22 @@ allOf:
         '#phy-cells':
           const: 1
           description: CPSW port number (starting from 1)
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - ti,j7200-cpsw5g-phy-gmii-sel
+    then:
+      properties:
+        '#phy-cells':
+          const: 1
+          description: CPSW port number (starting from 1)
+        ti,qsgmii-main-ports:
+          maxItems: 1
+    else:
+      properties:
+        ti,qsgmii-main-ports: false
   - if:
       properties:
         compatible:
@@ -97,7 +125,7 @@ additionalProperties: false
 
 examples:
   - |
-    phy_gmii_sel: phy-gmii-sel@650 {
+    phy_gmii_sel: phy@650 {
         compatible = "ti,am3352-phy-gmii-sel";
         reg = <0x650 0x4>;
         #phy-cells = <2>;
-- 
2.25.1

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

[Rob Herring]

On Mon, Aug 22, 2022 at 12:26:30PM +0530, Siddharth Vadapalli wrote:
> TI's J7200 SoC supports additional PHY modes like QSGMII and SGMII
> that are not supported on earlier SoCs. Add a compatible for it.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  .../mfd/ti,j721e-system-controller.yaml       |  6 ++++
>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 30 ++++++++++++++++++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
> index 73cffc45e056..466724cb4157 100644
> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
> @@ -54,6 +54,12 @@ patternProperties:
>      description:
>        Clock provider for TI EHRPWM nodes.
>  
> +  "phy@[0-9a-f]+$":
> +    type: object
> +    $ref: ../phy/ti,phy-gmii-sel.yaml

/schemas/phy/...

> +    description:
> +      This is the register to set phy mode through phy-gmii-sel driver.
> +
>  required:
>    - compatible
>    - reg
> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> index ff8a6d9eb153..0ffb97f1a77c 100644
> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> @@ -53,12 +53,24 @@ properties:
>        - ti,am43xx-phy-gmii-sel
>        - ti,dm814-phy-gmii-sel
>        - ti,am654-phy-gmii-sel
> +      - ti,j7200-cpsw5g-phy-gmii-sel
>  
>    reg:
>      maxItems: 1
>  
>    '#phy-cells': true
>  
> +  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.
> +    items:
> +      minimum: 1
> +      maximum: 4
> +
>  allOf:
>    - if:
>        properties:
> @@ -73,6 +85,22 @@ allOf:
>          '#phy-cells':
>            const: 1
>            description: CPSW port number (starting from 1)
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - ti,j7200-cpsw5g-phy-gmii-sel
> +    then:
> +      properties:
> +        '#phy-cells':
> +          const: 1
> +          description: CPSW port number (starting from 1)
> +        ti,qsgmii-main-ports:
> +          maxItems: 1

If the array size can only ever be 1, then it's a uint32, not a 
uint32-array.

> +    else:
> +      properties:
> +        ti,qsgmii-main-ports: false
>    - if:
>        properties:
>          compatible:
> @@ -97,7 +125,7 @@ additionalProperties: false
>  
>  examples:
>    - |
> -    phy_gmii_sel: phy-gmii-sel@650 {
> +    phy_gmii_sel: phy@650 {
>          compatible = "ti,am3352-phy-gmii-sel";
>          reg = <0x650 0x4>;
>          #phy-cells = <2>;
> -- 
> 2.25.1
> 
> 

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

[Siddharth Vadapalli]

Hello Rob,

On 23/08/22 03:11, Rob Herring wrote:
> On Mon, Aug 22, 2022 at 12:26:30PM +0530, Siddharth Vadapalli wrote:
>> TI's J7200 SoC supports additional PHY modes like QSGMII and SGMII
>> that are not supported on earlier SoCs. Add a compatible for it.
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  .../mfd/ti,j721e-system-controller.yaml       |  6 ++++
>>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 30 ++++++++++++++++++-
>>  2 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>> index 73cffc45e056..466724cb4157 100644
>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>> @@ -54,6 +54,12 @@ patternProperties:
>>      description:
>>        Clock provider for TI EHRPWM nodes.
>>  
>> +  "phy@[0-9a-f]+$":
>> +    type: object
>> +    $ref: ../phy/ti,phy-gmii-sel.yaml
> 
> /schemas/phy/...

Thank you for reviewing the patch. I will update $ref to
/schemas/phy/phy-provider.yaml.

> 
>> +    description:
>> +      This is the register to set phy mode through phy-gmii-sel driver.
>> +
>>  required:
>>    - compatible
>>    - reg
>> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> index ff8a6d9eb153..0ffb97f1a77c 100644
>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> @@ -53,12 +53,24 @@ properties:
>>        - ti,am43xx-phy-gmii-sel
>>        - ti,dm814-phy-gmii-sel
>>        - ti,am654-phy-gmii-sel
>> +      - ti,j7200-cpsw5g-phy-gmii-sel
>>  
>>    reg:
>>      maxItems: 1
>>  
>>    '#phy-cells': true
>>  
>> +  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.
>> +    items:
>> +      minimum: 1
>> +      maximum: 4
>> +
>>  allOf:
>>    - if:
>>        properties:
>> @@ -73,6 +85,22 @@ allOf:
>>          '#phy-cells':
>>            const: 1
>>            description: CPSW port number (starting from 1)
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - ti,j7200-cpsw5g-phy-gmii-sel
>> +    then:
>> +      properties:
>> +        '#phy-cells':
>> +          const: 1
>> +          description: CPSW port number (starting from 1)
>> +        ti,qsgmii-main-ports:
>> +          maxItems: 1
> 
> If the array size can only ever be 1, then it's a uint32, not a 
> uint32-array.

For the current device, there can be only one QSGMII main port and
uint32 will be sufficient. However, I plan to send patches for TI's
J721e device which supports up to 2 QSGMII main ports. I wish to
implement the property such that it can be reused by J721e. For this
reason, I am defining the property as a uint32-array. With this
implementation, J7200 will use the property as an array with one
element, while J721e will use the property as an array with two
elements. This is being done to avoid adding a new property in the
future just for J721e. Please let me know if this is fine.

Regards,
Siddharth.

[PATCH v3 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200

[Siddharth Vadapalli]

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

In TI's J7200, each of the CPSW5G ethernet interfaces can act as a
QSGMII or QSGMII-SUB port. The QSGMII 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.

To indicate the interface which will serve as the main QSGMII interface,
add a property "ti,qsgmii-main-ports", whose value indicates the
port number of the interface which shall serve as the main QSGMII
interface. The rest of the interfaces are then assigned QSGMII-SUB mode by
default.

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

diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
index d0ab69750c6b..270083606b14 100644
--- a/drivers/phy/ti/phy-gmii-sel.c
+++ b/drivers/phy/ti/phy-gmii-sel.c
@@ -22,6 +22,12 @@
 #define AM33XX_GMII_SEL_MODE_RMII	1
 #define AM33XX_GMII_SEL_MODE_RGMII	2
 
+/* J72xx SoC specific definitions for the CONTROL port */
+#define J72XX_GMII_SEL_MODE_QSGMII	4
+#define J72XX_GMII_SEL_MODE_QSGMII_SUB	6
+
+#define PHY_GMII_PORT(n)	BIT((n) - 1)
+
 enum {
 	PHY_GMII_SEL_PORT_MODE = 0,
 	PHY_GMII_SEL_RGMII_ID_MODE,
@@ -43,6 +49,7 @@ struct phy_gmii_sel_soc_data {
 	u32 features;
 	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
 	bool use_of_data;
+	u64 extra_modes;
 };
 
 struct phy_gmii_sel_priv {
@@ -53,6 +60,7 @@ struct phy_gmii_sel_priv {
 	struct phy_gmii_sel_phy_priv *if_phys;
 	u32 num_ports;
 	u32 reg_offset;
+	u32 qsgmii_main_ports;
 };
 
 static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
@@ -88,10 +96,17 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
 		gmii_sel_mode = AM33XX_GMII_SEL_MODE_MII;
 		break;
 
+	case PHY_INTERFACE_MODE_QSGMII:
+		if (!(soc_data->extra_modes & BIT(PHY_INTERFACE_MODE_QSGMII)))
+			goto unsupported;
+		if (if_phy->priv->qsgmii_main_ports & BIT(if_phy->id - 1))
+			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII;
+		else
+			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII_SUB;
+		break;
+
 	default:
-		dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
-			 if_phy->id, phy_modes(submode));
-		return -EINVAL;
+		goto unsupported;
 	}
 
 	if_phy->phy_if_mode = submode;
@@ -123,6 +138,11 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
 	}
 
 	return 0;
+
+unsupported:
+	dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
+		 if_phy->id, phy_modes(submode));
+	return -EINVAL;
 }
 
 static const
@@ -188,6 +208,13 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_soc_am654 = {
 	.regfields = phy_gmii_sel_fields_am654,
 };
 
+static const
+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),
+};
+
 static const struct of_device_id phy_gmii_sel_id_table[] = {
 	{
 		.compatible	= "ti,am3352-phy-gmii-sel",
@@ -209,6 +236,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = {
 		.compatible	= "ti,am654-phy-gmii-sel",
 		.data		= &phy_gmii_sel_soc_am654,
 	},
+	{
+		.compatible	= "ti,j7200-cpsw5g-phy-gmii-sel",
+		.data		= &phy_gmii_sel_cpsw5g_soc_j7200,
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table);
@@ -350,6 +381,7 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
 	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;
 
 	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
@@ -363,6 +395,8 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
 	priv->dev = &pdev->dev;
 	priv->soc_data = of_id->data;
 	priv->num_ports = priv->soc_data->num_ports;
+	of_property_read_u32_array(node, "ti,qsgmii-main-ports", &main_ports, 1);
+	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

Re: [PATCH v3 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200

[Roger Quadros]

Hi Siddharth,

On 22/08/2022 09:56, Siddharth Vadapalli wrote:
> Each of the CPSW5G ports in J7200 support additional modes like QSGMII.
> Add a new compatible for J7200 to support the additional modes.
> 
> In TI's J7200, each of the CPSW5G ethernet interfaces can act as a
> QSGMII or QSGMII-SUB port. The QSGMII 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.
> 
> To indicate the interface which will serve as the main QSGMII interface,
> add a property "ti,qsgmii-main-ports", whose value indicates the
> port number of the interface which shall serve as the main QSGMII
> interface. The rest of the interfaces are then assigned QSGMII-SUB mode by
> default.

Can you please describe here why you are using "ti,qsgmii-main-ports" instead
of "ti,qsgmii-main-port" as there can be only one main port per PHY instance?

> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  drivers/phy/ti/phy-gmii-sel.c | 40 ++++++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
> index d0ab69750c6b..270083606b14 100644
> --- a/drivers/phy/ti/phy-gmii-sel.c
> +++ b/drivers/phy/ti/phy-gmii-sel.c
> @@ -22,6 +22,12 @@
>  #define AM33XX_GMII_SEL_MODE_RMII	1
>  #define AM33XX_GMII_SEL_MODE_RGMII	2
>  
> +/* J72xx SoC specific definitions for the CONTROL port */
> +#define J72XX_GMII_SEL_MODE_QSGMII	4
> +#define J72XX_GMII_SEL_MODE_QSGMII_SUB	6
> +
> +#define PHY_GMII_PORT(n)	BIT((n) - 1)
> +
>  enum {
>  	PHY_GMII_SEL_PORT_MODE = 0,
>  	PHY_GMII_SEL_RGMII_ID_MODE,
> @@ -43,6 +49,7 @@ struct phy_gmii_sel_soc_data {
>  	u32 features;
>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>  	bool use_of_data;
> +	u64 extra_modes;
>  };
>  
>  struct phy_gmii_sel_priv {
> @@ -53,6 +60,7 @@ struct phy_gmii_sel_priv {
>  	struct phy_gmii_sel_phy_priv *if_phys;
>  	u32 num_ports;
>  	u32 reg_offset;
> +	u32 qsgmii_main_ports;
>  };
>  
>  static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
> @@ -88,10 +96,17 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>  		gmii_sel_mode = AM33XX_GMII_SEL_MODE_MII;
>  		break;
>  
> +	case PHY_INTERFACE_MODE_QSGMII:
> +		if (!(soc_data->extra_modes & BIT(PHY_INTERFACE_MODE_QSGMII)))
> +			goto unsupported;
> +		if (if_phy->priv->qsgmii_main_ports & BIT(if_phy->id - 1))
> +			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII;
> +		else
> +			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII_SUB;
> +		break;
> +
>  	default:
> -		dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
> -			 if_phy->id, phy_modes(submode));
> -		return -EINVAL;
> +		goto unsupported;
>  	}
>  
>  	if_phy->phy_if_mode = submode;
> @@ -123,6 +138,11 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>  	}
>  
>  	return 0;
> +
> +unsupported:
> +	dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
> +		 if_phy->id, phy_modes(submode));
> +	return -EINVAL;
>  }
>  
>  static const
> @@ -188,6 +208,13 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_soc_am654 = {
>  	.regfields = phy_gmii_sel_fields_am654,
>  };
>  
> +static const
> +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),
> +};
> +
>  static const struct of_device_id phy_gmii_sel_id_table[] = {
>  	{
>  		.compatible	= "ti,am3352-phy-gmii-sel",
> @@ -209,6 +236,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = {
>  		.compatible	= "ti,am654-phy-gmii-sel",
>  		.data		= &phy_gmii_sel_soc_am654,
>  	},
> +	{
> +		.compatible	= "ti,j7200-cpsw5g-phy-gmii-sel",
> +		.data		= &phy_gmii_sel_cpsw5g_soc_j7200,
> +	},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table);
> @@ -350,6 +381,7 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>  	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;
>  
>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
> @@ -363,6 +395,8 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>  	priv->dev = &pdev->dev;
>  	priv->soc_data = of_id->data;
>  	priv->num_ports = priv->soc_data->num_ports;
> +	of_property_read_u32_array(node, "ti,qsgmii-main-ports", &main_ports, 1);

of_property_read_u32_array can return error and you are not doing any sanity checks.
This should fail if "ti,qsgmii-main-ports" is not present or out of bounds if port mode is QSGMII.

How is this going to scale if you are going to have multiple main ports?
Let's say you increase it to 2 in the future. won't this break with -EOVERFLOW for older
DTs where you had only 1 u32.

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

cheers,
-roger

Re: [PATCH v3 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200

[Siddharth Vadapalli]

Hello Roger,

On 25/08/22 13:11, Roger Quadros wrote:
> Hi Siddharth,
> 
> On 22/08/2022 09:56, Siddharth Vadapalli wrote:
>> Each of the CPSW5G ports in J7200 support additional modes like QSGMII.
>> Add a new compatible for J7200 to support the additional modes.
>>
>> In TI's J7200, each of the CPSW5G ethernet interfaces can act as a
>> QSGMII or QSGMII-SUB port. The QSGMII 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.
>>
>> To indicate the interface which will serve as the main QSGMII interface,
>> add a property "ti,qsgmii-main-ports", whose value indicates the
>> port number of the interface which shall serve as the main QSGMII
>> interface. The rest of the interfaces are then assigned QSGMII-SUB mode by
>> default.
> 
> Can you please describe here why you are using "ti,qsgmii-main-ports" instead
> of "ti,qsgmii-main-port" as there can be only one main port per PHY instance?

Thank you for reviewing the patch. I am using "ports" instead of "port"
because I plan to add support for CPSW9G on TI's J721e device in the
future patches. CPSW9G (8 external ports) supports up to two QSGMII main
ports. For CPSW9G, by specifying the two main ports in the device tree,
it is possible to configure the CTRLMMR_ENETx_CTRL register for each of
the 8 ports, with the two QSGMII main ports being configured as main
ports in the CTRLMMR_ENETx_CTRL register and the rest of them being
configured as sub ports. Since I will be using the same property
"ti,qsgmii-main-ports" for CPSW9G as well, the property will be an array
of 2 values for CPSW9G. Therefore, I am using "ports" instead of "port".
Please let me know if this is fine.

> 
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  drivers/phy/ti/phy-gmii-sel.c | 40 ++++++++++++++++++++++++++++++++---
>>  1 file changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
>> index d0ab69750c6b..270083606b14 100644
>> --- a/drivers/phy/ti/phy-gmii-sel.c
>> +++ b/drivers/phy/ti/phy-gmii-sel.c
>> @@ -22,6 +22,12 @@
>>  #define AM33XX_GMII_SEL_MODE_RMII	1
>>  #define AM33XX_GMII_SEL_MODE_RGMII	2
>>  
>> +/* J72xx SoC specific definitions for the CONTROL port */
>> +#define J72XX_GMII_SEL_MODE_QSGMII	4
>> +#define J72XX_GMII_SEL_MODE_QSGMII_SUB	6
>> +
>> +#define PHY_GMII_PORT(n)	BIT((n) - 1)
>> +
>>  enum {
>>  	PHY_GMII_SEL_PORT_MODE = 0,
>>  	PHY_GMII_SEL_RGMII_ID_MODE,
>> @@ -43,6 +49,7 @@ struct phy_gmii_sel_soc_data {
>>  	u32 features;
>>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>>  	bool use_of_data;
>> +	u64 extra_modes;
>>  };
>>  
>>  struct phy_gmii_sel_priv {
>> @@ -53,6 +60,7 @@ struct phy_gmii_sel_priv {
>>  	struct phy_gmii_sel_phy_priv *if_phys;
>>  	u32 num_ports;
>>  	u32 reg_offset;
>> +	u32 qsgmii_main_ports;
>>  };
>>  
>>  static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>> @@ -88,10 +96,17 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>>  		gmii_sel_mode = AM33XX_GMII_SEL_MODE_MII;
>>  		break;
>>  
>> +	case PHY_INTERFACE_MODE_QSGMII:
>> +		if (!(soc_data->extra_modes & BIT(PHY_INTERFACE_MODE_QSGMII)))
>> +			goto unsupported;
>> +		if (if_phy->priv->qsgmii_main_ports & BIT(if_phy->id - 1))
>> +			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII;
>> +		else
>> +			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII_SUB;
>> +		break;
>> +
>>  	default:
>> -		dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
>> -			 if_phy->id, phy_modes(submode));
>> -		return -EINVAL;
>> +		goto unsupported;
>>  	}
>>  
>>  	if_phy->phy_if_mode = submode;
>> @@ -123,6 +138,11 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>>  	}
>>  
>>  	return 0;
>> +
>> +unsupported:
>> +	dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
>> +		 if_phy->id, phy_modes(submode));
>> +	return -EINVAL;
>>  }
>>  
>>  static const
>> @@ -188,6 +208,13 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_soc_am654 = {
>>  	.regfields = phy_gmii_sel_fields_am654,
>>  };
>>  
>> +static const
>> +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),
>> +};
>> +
>>  static const struct of_device_id phy_gmii_sel_id_table[] = {
>>  	{
>>  		.compatible	= "ti,am3352-phy-gmii-sel",
>> @@ -209,6 +236,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = {
>>  		.compatible	= "ti,am654-phy-gmii-sel",
>>  		.data		= &phy_gmii_sel_soc_am654,
>>  	},
>> +	{
>> +		.compatible	= "ti,j7200-cpsw5g-phy-gmii-sel",
>> +		.data		= &phy_gmii_sel_cpsw5g_soc_j7200,
>> +	},
>>  	{}
>>  };
>>  MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table);
>> @@ -350,6 +381,7 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>  	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;
>>  
>>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
>> @@ -363,6 +395,8 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>  	priv->dev = &pdev->dev;
>>  	priv->soc_data = of_id->data;
>>  	priv->num_ports = priv->soc_data->num_ports;
>> +	of_property_read_u32_array(node, "ti,qsgmii-main-ports", &main_ports, 1);
> 
> of_property_read_u32_array can return error and you are not doing any sanity checks.
> This should fail if "ti,qsgmii-main-ports" is not present or out of bounds if port mode is QSGMII.

In the scenario that the user doesn't want to use QSGMII mode, the
property will not be mentioned in the device tree. In the phy-gmii-sel
driver, the call to of_property_read_u32_array() will return an error
since the optional ti,qsgmii-main-ports property doesn't exist in the
device tree. However, the main_ports variable has already been
initialized to 1 and in case of Non-QSGMII modes, the main_ports
variable will not even be used. If the mode is QSGMII and the user
forgets to mention the ti,qsgmii-main-ports property in the device tree,
then the default value of 1 is used.

Since the of_property_read_u32_array() function doesn't overwrite
main_ports variable if the ti,qsgmii-main-ports property is not present
in the device tree, the value of main_ports will continue to remain 1
even in the case where of_property_read_u32_array() errors out.

In the other scenario where the user mentions a value that is smaller or
greater than the allowed value for "ti,qsgmii-main-ports" property, I
have added bindings to ensure that make dtbs_check fails. This will
enforce the bounds on the property.

For the above reasons, I think that the return value of the call to
of_property_read_u32_array() can be safely ignored, and the value of
main_ports doesn't need to be validated within the driver as it is being
enforced in the bindings.

> 
> How is this going to scale if you are going to have multiple main ports?
> Let's say you increase it to 2 in the future. won't this break with -EOVERFLOW for older
> DTs where you had only 1 u32.

For multiple main ports (like CPSW9G for example), I had planned to add
an IF-ELSE condition to check the compatible (I plan to add a new
compatible for J721e which uses CPSW9G) and then call
of_property_read_u32_array() with either 1 or 2 values to be read based
on the compatible.

Please let me know if you have a suggestion for a better way to
implement this.

Regards,
Siddharth.

Re: [PATCH v3 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200

[Roger Quadros]

Siddharth,

On 29/08/2022 07:53, Siddharth Vadapalli wrote:
> Hello Roger,
> 
> On 25/08/22 13:11, Roger Quadros wrote:
>> Hi Siddharth,
>>
>> On 22/08/2022 09:56, Siddharth Vadapalli wrote:
>>> Each of the CPSW5G ports in J7200 support additional modes like QSGMII.
>>> Add a new compatible for J7200 to support the additional modes.
>>>
>>> In TI's J7200, each of the CPSW5G ethernet interfaces can act as a
>>> QSGMII or QSGMII-SUB port. The QSGMII 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.
>>>
>>> To indicate the interface which will serve as the main QSGMII interface,
>>> add a property "ti,qsgmii-main-ports", whose value indicates the
>>> port number of the interface which shall serve as the main QSGMII
>>> interface. The rest of the interfaces are then assigned QSGMII-SUB mode by
>>> default.
>>
>> Can you please describe here why you are using "ti,qsgmii-main-ports" instead
>> of "ti,qsgmii-main-port" as there can be only one main port per PHY instance?
> 
> Thank you for reviewing the patch. I am using "ports" instead of "port"
> because I plan to add support for CPSW9G on TI's J721e device in the
> future patches. CPSW9G (8 external ports) supports up to two QSGMII main
> ports. For CPSW9G, by specifying the two main ports in the device tree,
> it is possible to configure the CTRLMMR_ENETx_CTRL register for each of
> the 8 ports, with the two QSGMII main ports being configured as main
> ports in the CTRLMMR_ENETx_CTRL register and the rest of them being
> configured as sub ports. Since I will be using the same property
> "ti,qsgmii-main-ports" for CPSW9G as well, the property will be an array
> of 2 values for CPSW9G. Therefore, I am using "ports" instead of "port".
> Please let me know if this is fine.
> 

OK. Please mention this in commit message.

>>
>>>
>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>> ---
>>>  drivers/phy/ti/phy-gmii-sel.c | 40 ++++++++++++++++++++++++++++++++---
>>>  1 file changed, 37 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
>>> index d0ab69750c6b..270083606b14 100644
>>> --- a/drivers/phy/ti/phy-gmii-sel.c
>>> +++ b/drivers/phy/ti/phy-gmii-sel.c
>>> @@ -22,6 +22,12 @@
>>>  #define AM33XX_GMII_SEL_MODE_RMII	1
>>>  #define AM33XX_GMII_SEL_MODE_RGMII	2
>>>  
>>> +/* J72xx SoC specific definitions for the CONTROL port */
>>> +#define J72XX_GMII_SEL_MODE_QSGMII	4
>>> +#define J72XX_GMII_SEL_MODE_QSGMII_SUB	6
>>> +
>>> +#define PHY_GMII_PORT(n)	BIT((n) - 1)
>>> +
>>>  enum {
>>>  	PHY_GMII_SEL_PORT_MODE = 0,
>>>  	PHY_GMII_SEL_RGMII_ID_MODE,
>>> @@ -43,6 +49,7 @@ struct phy_gmii_sel_soc_data {
>>>  	u32 features;
>>>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>>>  	bool use_of_data;
>>> +	u64 extra_modes;
>>>  };
>>>  
>>>  struct phy_gmii_sel_priv {
>>> @@ -53,6 +60,7 @@ struct phy_gmii_sel_priv {
>>>  	struct phy_gmii_sel_phy_priv *if_phys;
>>>  	u32 num_ports;
>>>  	u32 reg_offset;
>>> +	u32 qsgmii_main_ports;
>>>  };
>>>  
>>>  static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>>> @@ -88,10 +96,17 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>>>  		gmii_sel_mode = AM33XX_GMII_SEL_MODE_MII;
>>>  		break;
>>>  
>>> +	case PHY_INTERFACE_MODE_QSGMII:
>>> +		if (!(soc_data->extra_modes & BIT(PHY_INTERFACE_MODE_QSGMII)))
>>> +			goto unsupported;
>>> +		if (if_phy->priv->qsgmii_main_ports & BIT(if_phy->id - 1))
>>> +			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII;
>>> +		else
>>> +			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII_SUB;
>>> +		break;
>>> +
>>>  	default:
>>> -		dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
>>> -			 if_phy->id, phy_modes(submode));
>>> -		return -EINVAL;
>>> +		goto unsupported;
>>>  	}
>>>  
>>>  	if_phy->phy_if_mode = submode;
>>> @@ -123,6 +138,11 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>>>  	}
>>>  
>>>  	return 0;
>>> +
>>> +unsupported:
>>> +	dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
>>> +		 if_phy->id, phy_modes(submode));
>>> +	return -EINVAL;
>>>  }
>>>  
>>>  static const
>>> @@ -188,6 +208,13 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_soc_am654 = {
>>>  	.regfields = phy_gmii_sel_fields_am654,
>>>  };
>>>  
>>> +static const
>>> +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),
>>> +};
>>> +
>>>  static const struct of_device_id phy_gmii_sel_id_table[] = {
>>>  	{
>>>  		.compatible	= "ti,am3352-phy-gmii-sel",
>>> @@ -209,6 +236,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = {
>>>  		.compatible	= "ti,am654-phy-gmii-sel",
>>>  		.data		= &phy_gmii_sel_soc_am654,
>>>  	},
>>> +	{
>>> +		.compatible	= "ti,j7200-cpsw5g-phy-gmii-sel",
>>> +		.data		= &phy_gmii_sel_cpsw5g_soc_j7200,
>>> +	},
>>>  	{}
>>>  };
>>>  MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table);
>>> @@ -350,6 +381,7 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>>  	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;
>>>  
>>>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
>>> @@ -363,6 +395,8 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>>  	priv->dev = &pdev->dev;
>>>  	priv->soc_data = of_id->data;
>>>  	priv->num_ports = priv->soc_data->num_ports;
>>> +	of_property_read_u32_array(node, "ti,qsgmii-main-ports", &main_ports, 1);
>>
>> of_property_read_u32_array can return error and you are not doing any sanity checks.
>> This should fail if "ti,qsgmii-main-ports" is not present or out of bounds if port mode is QSGMII.
> 
> In the scenario that the user doesn't want to use QSGMII mode, the
> property will not be mentioned in the device tree. In the phy-gmii-sel
> driver, the call to of_property_read_u32_array() will return an error
> since the optional ti,qsgmii-main-ports property doesn't exist in the
> device tree. However, the main_ports variable has already been
> initialized to 1 and in case of Non-QSGMII modes, the main_ports
> variable will not even be used. If the mode is QSGMII and the user
> forgets to mention the ti,qsgmii-main-ports property in the device tree,
> then the default value of 1 is used.
> 
> Since the of_property_read_u32_array() function doesn't overwrite
> main_ports variable if the ti,qsgmii-main-ports property is not present
> in the device tree, the value of main_ports will continue to remain 1
> even in the case where of_property_read_u32_array() errors out.
> 
> In the other scenario where the user mentions a value that is smaller or
> greater than the allowed value for "ti,qsgmii-main-ports" property, I
> have added bindings to ensure that make dtbs_check fails. This will
> enforce the bounds on the property.

This I'm not sure about. dtbs_check is not always run at build time and we cannot
depend on that.

> 
> For the above reasons, I think that the return value of the call to
> of_property_read_u32_array() can be safely ignored, and the value of
> main_ports doesn't need to be validated within the driver as it is being
> enforced in the bindings.
> 
>>
>> How is this going to scale if you are going to have multiple main ports?
>> Let's say you increase it to 2 in the future. won't this break with -EOVERFLOW for older
>> DTs where you had only 1 u32.
> 
> For multiple main ports (like CPSW9G for example), I had planned to add
> an IF-ELSE condition to check the compatible (I plan to add a new
> compatible for J721e which uses CPSW9G) and then call
> of_property_read_u32_array() with either 1 or 2 values to be read based
> on the compatible.

In that case please use of_property_read_u32 for the case where you know only there is only 1 value.

> 
> Please let me know if you have a suggestion for a better way to
> implement this.
> 
> Regards,
> Siddharth.

cheers,
-roger

Re: [PATCH v3 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200

[Siddharth Vadapalli]

Hello Roger,

On 29/08/22 18:13, Roger Quadros wrote:
> Siddharth,
> 
> On 29/08/2022 07:53, Siddharth Vadapalli wrote:
>> Hello Roger,
>>
>> On 25/08/22 13:11, Roger Quadros wrote:
>>> Hi Siddharth,
>>>
>>> On 22/08/2022 09:56, Siddharth Vadapalli wrote:
>>>> Each of the CPSW5G ports in J7200 support additional modes like QSGMII.
>>>> Add a new compatible for J7200 to support the additional modes.
>>>>
>>>> In TI's J7200, each of the CPSW5G ethernet interfaces can act as a
>>>> QSGMII or QSGMII-SUB port. The QSGMII 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.
>>>>
>>>> To indicate the interface which will serve as the main QSGMII interface,
>>>> add a property "ti,qsgmii-main-ports", whose value indicates the
>>>> port number of the interface which shall serve as the main QSGMII
>>>> interface. The rest of the interfaces are then assigned QSGMII-SUB mode by
>>>> default.
>>>
>>> Can you please describe here why you are using "ti,qsgmii-main-ports" instead
>>> of "ti,qsgmii-main-port" as there can be only one main port per PHY instance?
>>
>> Thank you for reviewing the patch. I am using "ports" instead of "port"
>> because I plan to add support for CPSW9G on TI's J721e device in the
>> future patches. CPSW9G (8 external ports) supports up to two QSGMII main
>> ports. For CPSW9G, by specifying the two main ports in the device tree,
>> it is possible to configure the CTRLMMR_ENETx_CTRL register for each of
>> the 8 ports, with the two QSGMII main ports being configured as main
>> ports in the CTRLMMR_ENETx_CTRL register and the rest of them being
>> configured as sub ports. Since I will be using the same property
>> "ti,qsgmii-main-ports" for CPSW9G as well, the property will be an array
>> of 2 values for CPSW9G. Therefore, I am using "ports" instead of "port".
>> Please let me know if this is fine.
>>
> 
> OK. Please mention this in commit message.

I will mention that the property ti,qsgmii-main-ports is used to
configure the CTRLMMR_ENETx_CTRL register and that it is possible
depending on the device for there to be more than one main port which is
why the property is an array of values.

Would it be sufficient to mention the above in the commit message?
Please let me know.

> 
>>>
>>>>
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>> ---
>>>>  drivers/phy/ti/phy-gmii-sel.c | 40 ++++++++++++++++++++++++++++++++---
>>>>  1 file changed, 37 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
>>>> index d0ab69750c6b..270083606b14 100644
>>>> --- a/drivers/phy/ti/phy-gmii-sel.c
>>>> +++ b/drivers/phy/ti/phy-gmii-sel.c
>>>> @@ -22,6 +22,12 @@
>>>>  #define AM33XX_GMII_SEL_MODE_RMII	1
>>>>  #define AM33XX_GMII_SEL_MODE_RGMII	2
>>>>  
>>>> +/* J72xx SoC specific definitions for the CONTROL port */
>>>> +#define J72XX_GMII_SEL_MODE_QSGMII	4
>>>> +#define J72XX_GMII_SEL_MODE_QSGMII_SUB	6
>>>> +
>>>> +#define PHY_GMII_PORT(n)	BIT((n) - 1)
>>>> +
>>>>  enum {
>>>>  	PHY_GMII_SEL_PORT_MODE = 0,
>>>>  	PHY_GMII_SEL_RGMII_ID_MODE,
>>>> @@ -43,6 +49,7 @@ struct phy_gmii_sel_soc_data {
>>>>  	u32 features;
>>>>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>>>>  	bool use_of_data;
>>>> +	u64 extra_modes;
>>>>  };
>>>>  
>>>>  struct phy_gmii_sel_priv {
>>>> @@ -53,6 +60,7 @@ struct phy_gmii_sel_priv {
>>>>  	struct phy_gmii_sel_phy_priv *if_phys;
>>>>  	u32 num_ports;
>>>>  	u32 reg_offset;
>>>> +	u32 qsgmii_main_ports;
>>>>  };
>>>>  
>>>>  static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>>>> @@ -88,10 +96,17 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>>>>  		gmii_sel_mode = AM33XX_GMII_SEL_MODE_MII;
>>>>  		break;
>>>>  
>>>> +	case PHY_INTERFACE_MODE_QSGMII:
>>>> +		if (!(soc_data->extra_modes & BIT(PHY_INTERFACE_MODE_QSGMII)))
>>>> +			goto unsupported;
>>>> +		if (if_phy->priv->qsgmii_main_ports & BIT(if_phy->id - 1))
>>>> +			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII;
>>>> +		else
>>>> +			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII_SUB;
>>>> +		break;
>>>> +
>>>>  	default:
>>>> -		dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
>>>> -			 if_phy->id, phy_modes(submode));
>>>> -		return -EINVAL;
>>>> +		goto unsupported;
>>>>  	}
>>>>  
>>>>  	if_phy->phy_if_mode = submode;
>>>> @@ -123,6 +138,11 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>>>>  	}
>>>>  
>>>>  	return 0;
>>>> +
>>>> +unsupported:
>>>> +	dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
>>>> +		 if_phy->id, phy_modes(submode));
>>>> +	return -EINVAL;
>>>>  }
>>>>  
>>>>  static const
>>>> @@ -188,6 +208,13 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_soc_am654 = {
>>>>  	.regfields = phy_gmii_sel_fields_am654,
>>>>  };
>>>>  
>>>> +static const
>>>> +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),
>>>> +};
>>>> +
>>>>  static const struct of_device_id phy_gmii_sel_id_table[] = {
>>>>  	{
>>>>  		.compatible	= "ti,am3352-phy-gmii-sel",
>>>> @@ -209,6 +236,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = {
>>>>  		.compatible	= "ti,am654-phy-gmii-sel",
>>>>  		.data		= &phy_gmii_sel_soc_am654,
>>>>  	},
>>>> +	{
>>>> +		.compatible	= "ti,j7200-cpsw5g-phy-gmii-sel",
>>>> +		.data		= &phy_gmii_sel_cpsw5g_soc_j7200,
>>>> +	},
>>>>  	{}
>>>>  };
>>>>  MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table);
>>>> @@ -350,6 +381,7 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>>>  	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;
>>>>  
>>>>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
>>>> @@ -363,6 +395,8 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>>>  	priv->dev = &pdev->dev;
>>>>  	priv->soc_data = of_id->data;
>>>>  	priv->num_ports = priv->soc_data->num_ports;
>>>> +	of_property_read_u32_array(node, "ti,qsgmii-main-ports", &main_ports, 1);
>>>
>>> of_property_read_u32_array can return error and you are not doing any sanity checks.
>>> This should fail if "ti,qsgmii-main-ports" is not present or out of bounds if port mode is QSGMII.
>>
>> In the scenario that the user doesn't want to use QSGMII mode, the
>> property will not be mentioned in the device tree. In the phy-gmii-sel
>> driver, the call to of_property_read_u32_array() will return an error
>> since the optional ti,qsgmii-main-ports property doesn't exist in the
>> device tree. However, the main_ports variable has already been
>> initialized to 1 and in case of Non-QSGMII modes, the main_ports
>> variable will not even be used. If the mode is QSGMII and the user
>> forgets to mention the ti,qsgmii-main-ports property in the device tree,
>> then the default value of 1 is used.
>>
>> Since the of_property_read_u32_array() function doesn't overwrite
>> main_ports variable if the ti,qsgmii-main-ports property is not present
>> in the device tree, the value of main_ports will continue to remain 1
>> even in the case where of_property_read_u32_array() errors out.
>>
>> In the other scenario where the user mentions a value that is smaller or
>> greater than the allowed value for "ti,qsgmii-main-ports" property, I
>> have added bindings to ensure that make dtbs_check fails. This will
>> enforce the bounds on the property.
> 
> This I'm not sure about. dtbs_check is not always run at build time and we cannot
> depend on that.

If that's the case, then I will enforce a check in the phy-gmii-sel
driver as well. Thank you for letting me know.

> 
>>
>> For the above reasons, I think that the return value of the call to
>> of_property_read_u32_array() can be safely ignored, and the value of
>> main_ports doesn't need to be validated within the driver as it is being
>> enforced in the bindings.
>>
>>>
>>> How is this going to scale if you are going to have multiple main ports?
>>> Let's say you increase it to 2 in the future. won't this break with -EOVERFLOW for older
>>> DTs where you had only 1 u32.
>>
>> For multiple main ports (like CPSW9G for example), I had planned to add
>> an IF-ELSE condition to check the compatible (I plan to add a new
>> compatible for J721e which uses CPSW9G) and then call
>> of_property_read_u32_array() with either 1 or 2 values to be read based
>> on the compatible.
> 
> In that case please use of_property_read_u32 for the case where you know only there is only 1 value.

Yes I will do that. Thank you for reviewing the patch.

Regards,
Siddharth.

Re: [PATCH v3 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200

[Roger Quadros]

Siddharth,

On 30/08/2022 07:31, Siddharth Vadapalli wrote:
> Hello Roger,
> 
> On 29/08/22 18:13, Roger Quadros wrote:
>> Siddharth,
>>
>> On 29/08/2022 07:53, Siddharth Vadapalli wrote:
>>> Hello Roger,
>>>
>>> On 25/08/22 13:11, Roger Quadros wrote:
>>>> Hi Siddharth,
>>>>
>>>> On 22/08/2022 09:56, Siddharth Vadapalli wrote:
>>>>> Each of the CPSW5G ports in J7200 support additional modes like QSGMII.
>>>>> Add a new compatible for J7200 to support the additional modes.
>>>>>
>>>>> In TI's J7200, each of the CPSW5G ethernet interfaces can act as a
>>>>> QSGMII or QSGMII-SUB port. The QSGMII 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.
>>>>>
>>>>> To indicate the interface which will serve as the main QSGMII interface,
>>>>> add a property "ti,qsgmii-main-ports", whose value indicates the
>>>>> port number of the interface which shall serve as the main QSGMII
>>>>> interface. The rest of the interfaces are then assigned QSGMII-SUB mode by
>>>>> default.
>>>>
>>>> Can you please describe here why you are using "ti,qsgmii-main-ports" instead
>>>> of "ti,qsgmii-main-port" as there can be only one main port per PHY instance?
>>>
>>> Thank you for reviewing the patch. I am using "ports" instead of "port"
>>> because I plan to add support for CPSW9G on TI's J721e device in the
>>> future patches. CPSW9G (8 external ports) supports up to two QSGMII main
>>> ports. For CPSW9G, by specifying the two main ports in the device tree,
>>> it is possible to configure the CTRLMMR_ENETx_CTRL register for each of
>>> the 8 ports, with the two QSGMII main ports being configured as main
>>> ports in the CTRLMMR_ENETx_CTRL register and the rest of them being
>>> configured as sub ports. Since I will be using the same property
>>> "ti,qsgmii-main-ports" for CPSW9G as well, the property will be an array
>>> of 2 values for CPSW9G. Therefore, I am using "ports" instead of "port".
>>> Please let me know if this is fine.
>>>
>>
>> OK. Please mention this in commit message.
> 
> I will mention that the property ti,qsgmii-main-ports is used to
> configure the CTRLMMR_ENETx_CTRL register and that it is possible
> depending on the device for there to be more than one main port which is
> why the property is an array of values.
> 
> Would it be sufficient to mention the above in the commit message?
> Please let me know.

This is fine. Thanks.

cheers,
-roger