[PATCH 0/2] Add support for QSGMII mode

[PATCH 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.

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       |  5 +++
 .../bindings/phy/ti,phy-gmii-sel.yaml         | 24 +++++++++++-
 drivers/phy/ti/phy-gmii-sel.c                 | 39 +++++++++++++++++--
 3 files changed, 64 insertions(+), 4 deletions(-)

-- 
2.36.1

[PATCH 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       |  5 ++++
 .../bindings/phy/ti,phy-gmii-sel.yaml         | 24 ++++++++++++++++++-
 2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644
--- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
+++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
@@ -48,6 +48,11 @@ patternProperties:
     description:
       This is the SERDES lane control mux.
 
+  "phy@[0-9a-f]+$":
+    type: object
+    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..7427758451e7 100644
--- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
@@ -53,12 +53,21 @@ 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,enet-ctrl-qsgmii:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Required only for QSGMII mode. Bitmask to select the port for
+      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
+      ports automatically. Any of the 4 CPSW5G ports can act as the
+      main port with the rest of them being the QSGMII_SUB ports.
+
 allOf:
   - if:
       properties:
@@ -73,6 +82,19 @@ 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,enet-ctrl-qsgmii:
+          enum: [1, 2, 4, 8]
   - if:
       properties:
         compatible:
@@ -97,7 +119,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.36.1

[PATCH 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,enet-ctrl-qsgmii", 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 | 39 ++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
index d0ab69750c6b..3def0909ba69 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_port;
 };
 
 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_port & 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);
@@ -363,6 +394,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;
+	priv->qsgmii_main_port = PHY_GMII_PORT(1);
+	of_property_read_u32(node, "ti,enet-ctrl-qsgmii", &priv->qsgmii_main_port);
 
 	priv->regmap = syscon_node_to_regmap(node->parent);
 	if (IS_ERR(priv->regmap)) {
-- 
2.36.1

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

[Roger Quadros]

Hi Siddharth,

On 31/05/2022 14:12, 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       |  5 ++++
>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 24 ++++++++++++++++++-
>  2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644
> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
> @@ -48,6 +48,11 @@ patternProperties:
>      description:
>        This is the SERDES lane control mux.
>  
> +  "phy@[0-9a-f]+$":
> +    type: object
> +    description:
> +      This is the register to set phy mode through phy-gmii-sel driver.
> +

Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all.

>  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..7427758451e7 100644
> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> @@ -53,12 +53,21 @@ properties:
>        - ti,am43xx-phy-gmii-sel
>        - ti,dm814-phy-gmii-sel
>        - ti,am654-phy-gmii-sel
> +      - ti,j7200-cpsw5g-phy-gmii-sel

Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming.

>  
>    reg:
>      maxItems: 1
>  
>    '#phy-cells': true
>  
> +  ti,enet-ctrl-qsgmii:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Required only for QSGMII mode. Bitmask to select the port for
> +      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
> +      ports automatically. Any of the 4 CPSW5G ports can act as the
> +      main port with the rest of them being the QSGMII_SUB ports.
> +

This is weird way of doing things.

The Ethernet controller driver already knows which mode the port is
supposed to operate.

e.g.
+&cpsw0_port1 {
+	phy-handle = <&cpsw5g_phy0>;
+	phy-mode = "qsgmii";
+	mac-address = [00 00 00 00 00 00];
+	phys = <&cpsw0_phy_gmii_sel 1>;
+};
+
+&cpsw0_port2 {
+	phy-handle = <&cpsw5g_phy1>;
+	phy-mode = "qsgmii-sub";
+	mac-address = [00 00 00 00 00 00];
+	phys = <&cpsw0_phy_gmii_sel 2>;

And it can convey the mode to the PHY driver via phy_ops->set_mode.
So you should be depending on that instead of adding this new property.

>  allOf:
>    - if:
>        properties:
> @@ -73,6 +82,19 @@ 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,enet-ctrl-qsgmii:
> +          enum: [1, 2, 4, 8]
>    - if:
>        properties:
>          compatible:
> @@ -97,7 +119,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>;

cheers,
-roger

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

[Siddharth Vadapalli]

Hello Roger,

On 31/05/22 17:15, Roger Quadros wrote:
> Hi Siddharth,
> 
> On 31/05/2022 14:12, 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       |  5 ++++
>>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 24 ++++++++++++++++++-
>>  2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644
>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>> @@ -48,6 +48,11 @@ patternProperties:
>>      description:
>>        This is the SERDES lane control mux.
>>  
>> +  "phy@[0-9a-f]+$":
>> +    type: object
>> +    description:
>> +      This is the register to set phy mode through phy-gmii-sel driver.
>> +
> 
> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all.

The property has to be mentioned in order to pass: make dtbs_check.

> 
>>  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..7427758451e7 100644
>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> @@ -53,12 +53,21 @@ properties:
>>        - ti,am43xx-phy-gmii-sel
>>        - ti,dm814-phy-gmii-sel
>>        - ti,am654-phy-gmii-sel
>> +      - ti,j7200-cpsw5g-phy-gmii-sel
> 
> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming.

In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While
CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added
with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for
CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name
of the compatible.

> 
>>  
>>    reg:
>>      maxItems: 1
>>  
>>    '#phy-cells': true
>>  
>> +  ti,enet-ctrl-qsgmii:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: |
>> +      Required only for QSGMII mode. Bitmask to select the port for
>> +      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>> +      ports automatically. Any of the 4 CPSW5G ports can act as the
>> +      main port with the rest of them being the QSGMII_SUB ports.
>> +
> 
> This is weird way of doing things.
> 
> The Ethernet controller driver already knows which mode the port is
> supposed to operate.

From the ethernet driver perspective, there is no difference between the QSGMII
or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel
driver configures CPSW MAC registers differently depending on the mode being
QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to
identify the QSGMII main port and the rest are configured in CPSW MAC as
QSGMII-SUB ports.

> 
> e.g.
> +&cpsw0_port1 {
> +	phy-handle = <&cpsw5g_phy0>;
> +	phy-mode = "qsgmii";
> +	mac-address = [00 00 00 00 00 00];
> +	phys = <&cpsw0_phy_gmii_sel 1>;
> +};
> +
> +&cpsw0_port2 {
> +	phy-handle = <&cpsw5g_phy1>;
> +	phy-mode = "qsgmii-sub";
> +	mac-address = [00 00 00 00 00 00];
> +	phys = <&cpsw0_phy_gmii_sel 2>;
> 
> And it can convey the mode to the PHY driver via phy_ops->set_mode.
> So you should be depending on that instead of adding this new property.

QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with
the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB
has to be introduced to the kernel. Additionally, all existing phy drivers will
have to be updated to recognize the new phy mode.

Since the QSGMII-SUB mode is TI specific, it was decided that it would be better
to add a new property in TI specific files for identifying the QSGMII main port
and treating the rest as QSGMII-SUB ports.

Thanks,
Siddharth.

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

[Roger Quadros]

Siddharth,

On 01/06/2022 09:01, Siddharth Vadapalli wrote:
> Hello Roger,
> 
> On 31/05/22 17:15, Roger Quadros wrote:
>> Hi Siddharth,
>>
>> On 31/05/2022 14:12, 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       |  5 ++++
>>>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 24 ++++++++++++++++++-
>>>  2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644
>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>> @@ -48,6 +48,11 @@ patternProperties:
>>>      description:
>>>        This is the SERDES lane control mux.
>>>  
>>> +  "phy@[0-9a-f]+$":
>>> +    type: object
>>> +    description:
>>> +      This is the register to set phy mode through phy-gmii-sel driver.
>>> +
>>
>> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all.
> 
> The property has to be mentioned in order to pass: make dtbs_check.
> 
>>
>>>  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..7427758451e7 100644
>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>> @@ -53,12 +53,21 @@ properties:
>>>        - ti,am43xx-phy-gmii-sel
>>>        - ti,dm814-phy-gmii-sel
>>>        - ti,am654-phy-gmii-sel
>>> +      - ti,j7200-cpsw5g-phy-gmii-sel
>>
>> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming.
> 
> In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While
> CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added
> with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for
> CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name
> of the compatible.

Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G)
Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong.

You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode.

> 
>>
>>>  
>>>    reg:
>>>      maxItems: 1
>>>  
>>>    '#phy-cells': true
>>>  
>>> +  ti,enet-ctrl-qsgmii:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description: |
>>> +      Required only for QSGMII mode. Bitmask to select the port for
>>> +      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>>> +      ports automatically. Any of the 4 CPSW5G ports can act as the
>>> +      main port with the rest of them being the QSGMII_SUB ports.
>>> +
>>
>> This is weird way of doing things.
>>
>> The Ethernet controller driver already knows which mode the port is
>> supposed to operate.
> 
> From the ethernet driver perspective, there is no difference between the QSGMII
> or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel
> driver configures CPSW MAC registers differently depending on the mode being
> QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to
> identify the QSGMII main port and the rest are configured in CPSW MAC as
> QSGMII-SUB ports.
> 
>>
>> e.g.
>> +&cpsw0_port1 {
>> +	phy-handle = <&cpsw5g_phy0>;
>> +	phy-mode = "qsgmii";
>> +	mac-address = [00 00 00 00 00 00];
>> +	phys = <&cpsw0_phy_gmii_sel 1>;
>> +};
>> +
>> +&cpsw0_port2 {
>> +	phy-handle = <&cpsw5g_phy1>;
>> +	phy-mode = "qsgmii-sub";
>> +	mac-address = [00 00 00 00 00 00];
>> +	phys = <&cpsw0_phy_gmii_sel 2>;
>>
>> And it can convey the mode to the PHY driver via phy_ops->set_mode.
>> So you should be depending on that instead of adding this new property.
> 
> QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with
> the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB
> has to be introduced to the kernel. Additionally, all existing phy drivers will
> have to be updated to recognize the new phy mode.
> 
> Since the QSGMII-SUB mode is TI specific, it was decided that it would be better
> to add a new property in TI specific files for identifying the QSGMII main port
> and treating the rest as QSGMII-SUB ports.

Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN?
Can all ports be SUB or there has to be at least one MAIN?

Just trying to understand if there can be a solution without introducing a new
custom property.

cheers,
-roger

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

[Siddharth Vadapalli]

Hello Roger,

On 01/06/22 15:08, Roger Quadros wrote:
> Siddharth,
> 
> On 01/06/2022 09:01, Siddharth Vadapalli wrote:
>> Hello Roger,
>>
>> On 31/05/22 17:15, Roger Quadros wrote:
>>> Hi Siddharth,
>>>
>>> On 31/05/2022 14:12, 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       |  5 ++++
>>>>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 24 ++++++++++++++++++-
>>>>  2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644
>>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>>> @@ -48,6 +48,11 @@ patternProperties:
>>>>      description:
>>>>        This is the SERDES lane control mux.
>>>>  
>>>> +  "phy@[0-9a-f]+$":
>>>> +    type: object
>>>> +    description:
>>>> +      This is the register to set phy mode through phy-gmii-sel driver.
>>>> +
>>>
>>> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all.
>>
>> The property has to be mentioned in order to pass: make dtbs_check.
>>
>>>
>>>>  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..7427758451e7 100644
>>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>>> @@ -53,12 +53,21 @@ properties:
>>>>        - ti,am43xx-phy-gmii-sel
>>>>        - ti,dm814-phy-gmii-sel
>>>>        - ti,am654-phy-gmii-sel
>>>> +      - ti,j7200-cpsw5g-phy-gmii-sel
>>>
>>> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming.
>>
>> In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While
>> CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added
>> with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for
>> CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name
>> of the compatible.
> 
> Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G)
> Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong.

The PHY on J7200 is part of the Add-On Ethernet card. It is possible to connect
RGMII, QSGMII and SGMII PHY. The CPSW5G MAC supports all these modes. With the
current patch, I am adding just QSGMII mode as an extra mode, but in a future
patch, I will be adding SGMII also as an extra mode. For this reason, CPSW5G is
being mentioned in the compatible name, to differentiate supported modes for
CPSW2G and CPSW5G. Also, the phy-gmii-sel driver actually configures CPSW MAC
registers and not the PHY.

> 
> You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode.

Yes, I will add a check there too by using a different compatible in the CPSW
driver, but shouldn't the phy-gmii-sel driver also have a check to ensure that
it doesn't try configuring QSGMII mode for CPSW2G?

> 
>>
>>>
>>>>  
>>>>    reg:
>>>>      maxItems: 1
>>>>  
>>>>    '#phy-cells': true
>>>>  
>>>> +  ti,enet-ctrl-qsgmii:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description: |
>>>> +      Required only for QSGMII mode. Bitmask to select the port for
>>>> +      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>>>> +      ports automatically. Any of the 4 CPSW5G ports can act as the
>>>> +      main port with the rest of them being the QSGMII_SUB ports.
>>>> +
>>>
>>> This is weird way of doing things.
>>>
>>> The Ethernet controller driver already knows which mode the port is
>>> supposed to operate.
>>
>> From the ethernet driver perspective, there is no difference between the QSGMII
>> or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel
>> driver configures CPSW MAC registers differently depending on the mode being
>> QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to
>> identify the QSGMII main port and the rest are configured in CPSW MAC as
>> QSGMII-SUB ports.
>>
>>>
>>> e.g.
>>> +&cpsw0_port1 {
>>> +	phy-handle = <&cpsw5g_phy0>;
>>> +	phy-mode = "qsgmii";
>>> +	mac-address = [00 00 00 00 00 00];
>>> +	phys = <&cpsw0_phy_gmii_sel 1>;
>>> +};
>>> +
>>> +&cpsw0_port2 {
>>> +	phy-handle = <&cpsw5g_phy1>;
>>> +	phy-mode = "qsgmii-sub";
>>> +	mac-address = [00 00 00 00 00 00];
>>> +	phys = <&cpsw0_phy_gmii_sel 2>;
>>>
>>> And it can convey the mode to the PHY driver via phy_ops->set_mode.
>>> So you should be depending on that instead of adding this new property.
>>
>> QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with
>> the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB
>> has to be introduced to the kernel. Additionally, all existing phy drivers will
>> have to be updated to recognize the new phy mode.
>>
>> Since the QSGMII-SUB mode is TI specific, it was decided that it would be better
>> to add a new property in TI specific files for identifying the QSGMII main port
>> and treating the rest as QSGMII-SUB ports.
> 
> Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN?
> Can all ports be SUB or there has to be at least one MAIN?

All 4 ports in CPSW5G have the capability to be the MAIN port, with the only
restriction being that only one of them should be the MAIN port at a time. The
role of the CPSW5G ports is decided based on what PHY port each of the CPSW5G
ports connects to.

MAIN port of CPSW5G MAC is responsible for auto-negotiation with the PHY port on
the PHY which supports auto-negotiation. Thus, there can and should be only one
MAIN port.

Thanks,
Siddharth.

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

[Roger Quadros]

Hi Siddharth,

On 01/06/2022 14:27, Siddharth Vadapalli wrote:
> Hello Roger,
> 
> On 01/06/22 15:08, Roger Quadros wrote:
>> Siddharth,
>>
>> On 01/06/2022 09:01, Siddharth Vadapalli wrote:
>>> Hello Roger,
>>>
>>> On 31/05/22 17:15, Roger Quadros wrote:
>>>> Hi Siddharth,
>>>>
>>>> On 31/05/2022 14:12, 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       |  5 ++++
>>>>>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 24 ++++++++++++++++++-
>>>>>  2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644
>>>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>>>> @@ -48,6 +48,11 @@ patternProperties:
>>>>>      description:
>>>>>        This is the SERDES lane control mux.
>>>>>  
>>>>> +  "phy@[0-9a-f]+$":
>>>>> +    type: object
>>>>> +    description:
>>>>> +      This is the register to set phy mode through phy-gmii-sel driver.
>>>>> +
>>>>
>>>> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all.
>>>
>>> The property has to be mentioned in order to pass: make dtbs_check.
>>>
>>>>
>>>>>  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..7427758451e7 100644
>>>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>>>> @@ -53,12 +53,21 @@ properties:
>>>>>        - ti,am43xx-phy-gmii-sel
>>>>>        - ti,dm814-phy-gmii-sel
>>>>>        - ti,am654-phy-gmii-sel
>>>>> +      - ti,j7200-cpsw5g-phy-gmii-sel
>>>>
>>>> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming.
>>>
>>> In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While
>>> CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added
>>> with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for
>>> CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name
>>> of the compatible.
>>
>> Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G)
>> Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong.
> 
> The PHY on J7200 is part of the Add-On Ethernet card. It is possible to connect
> RGMII, QSGMII and SGMII PHY. The CPSW5G MAC supports all these modes. With the
> current patch, I am adding just QSGMII mode as an extra mode, but in a future
> patch, I will be adding SGMII also as an extra mode. For this reason, CPSW5G is
> being mentioned in the compatible name, to differentiate supported modes for
> CPSW2G and CPSW5G. Also, the phy-gmii-sel driver actually configures CPSW MAC
> registers and not the PHY.

phy-gmii-sel configures CTRL MMR register right? How does it configure CPSW MAC register?

Anyways, I just looked at the TRM and there are in fact separate phy-gmii-sel (ENET_CTRL)
registers for CPSW2g and CPSW5g. So they warrant for separate compatibles as they are
not identical.
> 
>>
>> You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode.
> 
> Yes, I will add a check there too by using a different compatible in the CPSW
> driver, but shouldn't the phy-gmii-sel driver also have a check to ensure that
> it doesn't try configuring QSGMII mode for CPSW2G?

Yes, additional check in phy-gmii-sel driver is fine.

> 
>>
>>>
>>>>
>>>>>  
>>>>>    reg:
>>>>>      maxItems: 1
>>>>>  
>>>>>    '#phy-cells': true
>>>>>  
>>>>> +  ti,enet-ctrl-qsgmii:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    description: |
>>>>> +      Required only for QSGMII mode. Bitmask to select the port for
>>>>> +      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>>>>> +      ports automatically. Any of the 4 CPSW5G ports can act as the
>>>>> +      main port with the rest of them being the QSGMII_SUB ports.
>>>>> +
>>>>
>>>> This is weird way of doing things.
>>>>
>>>> The Ethernet controller driver already knows which mode the port is
>>>> supposed to operate.
>>>
>>> From the ethernet driver perspective, there is no difference between the QSGMII
>>> or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel
>>> driver configures CPSW MAC registers differently depending on the mode being

You mean the ENET_CTRL register in CTRL_MMR space?

>>> QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to
>>> identify the QSGMII main port and the rest are configured in CPSW MAC as
>>> QSGMII-SUB ports.
>>>
>>>>
>>>> e.g.
>>>> +&cpsw0_port1 {
>>>> +	phy-handle = <&cpsw5g_phy0>;
>>>> +	phy-mode = "qsgmii";
>>>> +	mac-address = [00 00 00 00 00 00];
>>>> +	phys = <&cpsw0_phy_gmii_sel 1>;
>>>> +};
>>>> +
>>>> +&cpsw0_port2 {
>>>> +	phy-handle = <&cpsw5g_phy1>;
>>>> +	phy-mode = "qsgmii-sub";
>>>> +	mac-address = [00 00 00 00 00 00];
>>>> +	phys = <&cpsw0_phy_gmii_sel 2>;
>>>>
>>>> And it can convey the mode to the PHY driver via phy_ops->set_mode.
>>>> So you should be depending on that instead of adding this new property.
>>>
>>> QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with
>>> the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB
>>> has to be introduced to the kernel. Additionally, all existing phy drivers will
>>> have to be updated to recognize the new phy mode.
>>>
>>> Since the QSGMII-SUB mode is TI specific, it was decided that it would be better
>>> to add a new property in TI specific files for identifying the QSGMII main port
>>> and treating the rest as QSGMII-SUB ports.
>>
>> Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN?
>> Can all ports be SUB or there has to be at least one MAIN?
> 
> All 4 ports in CPSW5G have the capability to be the MAIN port, with the only
> restriction being that only one of them should be the MAIN port at a time. The
> role of the CPSW5G ports is decided based on what PHY port each of the CPSW5G
> ports connects to.

OK, then instead of using bitmask and property being named "ti,enet-ctrl-qsgmii", why not
just say "ti,qsgmii-main-port" = <main_port_number>;

Also do some sanity check when getting that property.

> 
> MAIN port of CPSW5G MAC is responsible for auto-negotiation with the PHY port on
> the PHY which supports auto-negotiation. Thus, there can and should be only one
> MAIN port.
> 
> Thanks,
> Siddharth.

cheers,
-roger

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

[Siddharth Vadapalli]

Hello Roger,

On 03/06/22 14:18, Roger Quadros wrote:
> Hi Siddharth,
> 
> On 01/06/2022 14:27, Siddharth Vadapalli wrote:
>> Hello Roger,
>>
>> On 01/06/22 15:08, Roger Quadros wrote:
>>> Siddharth,
>>>
>>> On 01/06/2022 09:01, Siddharth Vadapalli wrote:
>>>> Hello Roger,
>>>>
>>>> On 31/05/22 17:15, Roger Quadros wrote:
>>>>> Hi Siddharth,
>>>>>
>>>>> On 31/05/2022 14:12, 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       |  5 ++++
>>>>>>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 24 ++++++++++++++++++-
>>>>>>  2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644
>>>>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>>>>> @@ -48,6 +48,11 @@ patternProperties:
>>>>>>      description:
>>>>>>        This is the SERDES lane control mux.
>>>>>>  
>>>>>> +  "phy@[0-9a-f]+$":
>>>>>> +    type: object
>>>>>> +    description:
>>>>>> +      This is the register to set phy mode through phy-gmii-sel driver.
>>>>>> +
>>>>>
>>>>> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all.
>>>>
>>>> The property has to be mentioned in order to pass: make dtbs_check.
>>>>
>>>>>
>>>>>>  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..7427758451e7 100644
>>>>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>>>>> @@ -53,12 +53,21 @@ properties:
>>>>>>        - ti,am43xx-phy-gmii-sel
>>>>>>        - ti,dm814-phy-gmii-sel
>>>>>>        - ti,am654-phy-gmii-sel
>>>>>> +      - ti,j7200-cpsw5g-phy-gmii-sel
>>>>>
>>>>> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming.
>>>>
>>>> In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While
>>>> CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added
>>>> with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for
>>>> CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name
>>>> of the compatible.
>>>
>>> Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G)
>>> Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong.
>>
>> The PHY on J7200 is part of the Add-On Ethernet card. It is possible to connect
>> RGMII, QSGMII and SGMII PHY. The CPSW5G MAC supports all these modes. With the
>> current patch, I am adding just QSGMII mode as an extra mode, but in a future
>> patch, I will be adding SGMII also as an extra mode. For this reason, CPSW5G is
>> being mentioned in the compatible name, to differentiate supported modes for
>> CPSW2G and CPSW5G. Also, the phy-gmii-sel driver actually configures CPSW MAC
>> registers and not the PHY.
> 
> phy-gmii-sel configures CTRL MMR register right? How does it configure CPSW MAC register?
> 
> Anyways, I just looked at the TRM and there are in fact separate phy-gmii-sel (ENET_CTRL)
> registers for CPSW2g and CPSW5g. So they warrant for separate compatibles as they are
> not identical.

By CPSW MAC registers being configured, I meant that the configuration being
done is for the MAC and not for the PHY. As per the TRM, for CPSW2G, the
CTRLMMR_MCU_ENET_CTRL register is configured and for CPSW5G, the
CTRLMMR_ENETx_CTRL registers are configured, with x ranging from 1 to 4
(corresponding to the 4 ports of CPSW5G). These registers configure the CPSW MAC
(CPSW2G/CPSW5G) and not the PHY. For this reason, I think that it would be
appropriate to use cpsw5g in the compatible name, to indicate which CTRLMMR
registers are being configured.

>>
>>>
>>> You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode.
>>
>> Yes, I will add a check there too by using a different compatible in the CPSW
>> driver, but shouldn't the phy-gmii-sel driver also have a check to ensure that
>> it doesn't try configuring QSGMII mode for CPSW2G?
> 
> Yes, additional check in phy-gmii-sel driver is fine.
> 
>>
>>>
>>>>
>>>>>
>>>>>>  
>>>>>>    reg:
>>>>>>      maxItems: 1
>>>>>>  
>>>>>>    '#phy-cells': true
>>>>>>  
>>>>>> +  ti,enet-ctrl-qsgmii:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +    description: |
>>>>>> +      Required only for QSGMII mode. Bitmask to select the port for
>>>>>> +      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>>>>>> +      ports automatically. Any of the 4 CPSW5G ports can act as the
>>>>>> +      main port with the rest of them being the QSGMII_SUB ports.
>>>>>> +
>>>>>
>>>>> This is weird way of doing things.
>>>>>
>>>>> The Ethernet controller driver already knows which mode the port is
>>>>> supposed to operate.
>>>>
>>>> From the ethernet driver perspective, there is no difference between the QSGMII
>>>> or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel
>>>> driver configures CPSW MAC registers differently depending on the mode being
> 
> You mean the ENET_CTRL register in CTRL_MMR space?

Yes I am referring to the CTRLMMR_ENETx_CTRL registers as per the J7200 TRM,
corresponding to the CPSW5G MAC.

> 
>>>> QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to
>>>> identify the QSGMII main port and the rest are configured in CPSW MAC as
>>>> QSGMII-SUB ports.
>>>>
>>>>>
>>>>> e.g.
>>>>> +&cpsw0_port1 {
>>>>> +	phy-handle = <&cpsw5g_phy0>;
>>>>> +	phy-mode = "qsgmii";
>>>>> +	mac-address = [00 00 00 00 00 00];
>>>>> +	phys = <&cpsw0_phy_gmii_sel 1>;
>>>>> +};
>>>>> +
>>>>> +&cpsw0_port2 {
>>>>> +	phy-handle = <&cpsw5g_phy1>;
>>>>> +	phy-mode = "qsgmii-sub";
>>>>> +	mac-address = [00 00 00 00 00 00];
>>>>> +	phys = <&cpsw0_phy_gmii_sel 2>;
>>>>>
>>>>> And it can convey the mode to the PHY driver via phy_ops->set_mode.
>>>>> So you should be depending on that instead of adding this new property.
>>>>
>>>> QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with
>>>> the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB
>>>> has to be introduced to the kernel. Additionally, all existing phy drivers will
>>>> have to be updated to recognize the new phy mode.
>>>>
>>>> Since the QSGMII-SUB mode is TI specific, it was decided that it would be better
>>>> to add a new property in TI specific files for identifying the QSGMII main port
>>>> and treating the rest as QSGMII-SUB ports.
>>>
>>> Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN?
>>> Can all ports be SUB or there has to be at least one MAIN?
>>
>> All 4 ports in CPSW5G have the capability to be the MAIN port, with the only
>> restriction being that only one of them should be the MAIN port at a time. The
>> role of the CPSW5G ports is decided based on what PHY port each of the CPSW5G
>> ports connects to.
> 
> OK, then instead of using bitmask and property being named "ti,enet-ctrl-qsgmii", why not
> just say "ti,qsgmii-main-port" = <main_port_number>;

I plan to send patches for J721e device which has CPSW9G (8 external ports) MAC.
CPSW9G can work with two sets of QSGMII interfaces (4 ports + 4 ports). Thus,
using a bitmask for the QSGMII main port will help identify the QSGMII main port
across both sets of QSGMII interfaces. The bitmask in case of J721e CPSW9G will
consider the first 4 bits for the first interface's 4 ports and the next 4 bits
for the second interface's 4 ports. In this manner, it will be possible to
extend it for 8 port CPSW9G MAC as well, without having to add a new property
for the second QSGMII interface.

> 
> Also do some sanity check when getting that property.

To ensure that multiple QSGMII ports are not declared as the main port, the
"ti,enet-ctrl-qsgmii" property has been declared as an enum: [1,2,4,8]. If a
different value other than the value in enum were to be used, then "make
dtbs_check" would raise an error. This will prevent configuring multiple QSGMII
ports as the main port at once. Also, in the phy-gmii-sel driver, a default
value of 1 is being assigned to the variable that will store the value
corresponding to the ti,enet-ctrl-qsgmii property from the device tree, thereby
treating the first CPSW5G port as the QSGMII main port by default.

Thanks,
Siddharth.

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

[Roger Quadros]

Siddharth,

On 03/06/2022 13:49, Siddharth Vadapalli wrote:
> Hello Roger,
> 
> On 03/06/22 14:18, Roger Quadros wrote:
>> Hi Siddharth,
>>
>> On 01/06/2022 14:27, Siddharth Vadapalli wrote:
>>> Hello Roger,
>>>
>>> On 01/06/22 15:08, Roger Quadros wrote:
>>>> Siddharth,
>>>>
>>>> On 01/06/2022 09:01, Siddharth Vadapalli wrote:
>>>>> Hello Roger,
>>>>>
>>>>> On 31/05/22 17:15, Roger Quadros wrote:
>>>>>> Hi Siddharth,
>>>>>>
>>>>>> On 31/05/2022 14:12, 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       |  5 ++++
>>>>>>>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 24 ++++++++++++++++++-
>>>>>>>  2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644
>>>>>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>>>>>> @@ -48,6 +48,11 @@ patternProperties:
>>>>>>>      description:
>>>>>>>        This is the SERDES lane control mux.
>>>>>>>  
>>>>>>> +  "phy@[0-9a-f]+$":
>>>>>>> +    type: object
>>>>>>> +    description:
>>>>>>> +      This is the register to set phy mode through phy-gmii-sel driver.
>>>>>>> +
>>>>>>
>>>>>> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all.
>>>>>
>>>>> The property has to be mentioned in order to pass: make dtbs_check.
>>>>>
>>>>>>
>>>>>>>  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..7427758451e7 100644
>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>>>>>> @@ -53,12 +53,21 @@ properties:
>>>>>>>        - ti,am43xx-phy-gmii-sel
>>>>>>>        - ti,dm814-phy-gmii-sel
>>>>>>>        - ti,am654-phy-gmii-sel
>>>>>>> +      - ti,j7200-cpsw5g-phy-gmii-sel
>>>>>>
>>>>>> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming.
>>>>>
>>>>> In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While
>>>>> CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added
>>>>> with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for
>>>>> CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name
>>>>> of the compatible.
>>>>
>>>> Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G)
>>>> Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong.
>>>
>>> The PHY on J7200 is part of the Add-On Ethernet card. It is possible to connect
>>> RGMII, QSGMII and SGMII PHY. The CPSW5G MAC supports all these modes. With the
>>> current patch, I am adding just QSGMII mode as an extra mode, but in a future
>>> patch, I will be adding SGMII also as an extra mode. For this reason, CPSW5G is
>>> being mentioned in the compatible name, to differentiate supported modes for
>>> CPSW2G and CPSW5G. Also, the phy-gmii-sel driver actually configures CPSW MAC
>>> registers and not the PHY.
>>
>> phy-gmii-sel configures CTRL MMR register right? How does it configure CPSW MAC register?
>>
>> Anyways, I just looked at the TRM and there are in fact separate phy-gmii-sel (ENET_CTRL)
>> registers for CPSW2g and CPSW5g. So they warrant for separate compatibles as they are
>> not identical.
> 
> By CPSW MAC registers being configured, I meant that the configuration being
> done is for the MAC and not for the PHY. As per the TRM, for CPSW2G, the
> CTRLMMR_MCU_ENET_CTRL register is configured and for CPSW5G, the
> CTRLMMR_ENETx_CTRL registers are configured, with x ranging from 1 to 4
> (corresponding to the 4 ports of CPSW5G). These registers configure the CPSW MAC
> (CPSW2G/CPSW5G) and not the PHY. For this reason, I think that it would be
> appropriate to use cpsw5g in the compatible name, to indicate which CTRLMMR
> registers are being configured.

Yes, I already agreed that separate compatible is fine :).

> 
>>>
>>>>
>>>> You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode.
>>>
>>> Yes, I will add a check there too by using a different compatible in the CPSW
>>> driver, but shouldn't the phy-gmii-sel driver also have a check to ensure that
>>> it doesn't try configuring QSGMII mode for CPSW2G?
>>
>> Yes, additional check in phy-gmii-sel driver is fine.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>  
>>>>>>>    reg:
>>>>>>>      maxItems: 1
>>>>>>>  
>>>>>>>    '#phy-cells': true
>>>>>>>  
>>>>>>> +  ti,enet-ctrl-qsgmii:
>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>> +    description: |
>>>>>>> +      Required only for QSGMII mode. Bitmask to select the port for
>>>>>>> +      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>>>>>>> +      ports automatically. Any of the 4 CPSW5G ports can act as the
>>>>>>> +      main port with the rest of them being the QSGMII_SUB ports.
>>>>>>> +
>>>>>>
>>>>>> This is weird way of doing things.
>>>>>>
>>>>>> The Ethernet controller driver already knows which mode the port is
>>>>>> supposed to operate.
>>>>>
>>>>> From the ethernet driver perspective, there is no difference between the QSGMII
>>>>> or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel
>>>>> driver configures CPSW MAC registers differently depending on the mode being
>>
>> You mean the ENET_CTRL register in CTRL_MMR space?
> 
> Yes I am referring to the CTRLMMR_ENETx_CTRL registers as per the J7200 TRM,
> corresponding to the CPSW5G MAC.
> 
>>
>>>>> QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to
>>>>> identify the QSGMII main port and the rest are configured in CPSW MAC as
>>>>> QSGMII-SUB ports.
>>>>>
>>>>>>
>>>>>> e.g.
>>>>>> +&cpsw0_port1 {
>>>>>> +	phy-handle = <&cpsw5g_phy0>;
>>>>>> +	phy-mode = "qsgmii";
>>>>>> +	mac-address = [00 00 00 00 00 00];
>>>>>> +	phys = <&cpsw0_phy_gmii_sel 1>;
>>>>>> +};
>>>>>> +
>>>>>> +&cpsw0_port2 {
>>>>>> +	phy-handle = <&cpsw5g_phy1>;
>>>>>> +	phy-mode = "qsgmii-sub";
>>>>>> +	mac-address = [00 00 00 00 00 00];
>>>>>> +	phys = <&cpsw0_phy_gmii_sel 2>;
>>>>>>
>>>>>> And it can convey the mode to the PHY driver via phy_ops->set_mode.
>>>>>> So you should be depending on that instead of adding this new property.
>>>>>
>>>>> QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with
>>>>> the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB
>>>>> has to be introduced to the kernel. Additionally, all existing phy drivers will
>>>>> have to be updated to recognize the new phy mode.
>>>>>
>>>>> Since the QSGMII-SUB mode is TI specific, it was decided that it would be better
>>>>> to add a new property in TI specific files for identifying the QSGMII main port
>>>>> and treating the rest as QSGMII-SUB ports.
>>>>
>>>> Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN?
>>>> Can all ports be SUB or there has to be at least one MAIN?
>>>
>>> All 4 ports in CPSW5G have the capability to be the MAIN port, with the only
>>> restriction being that only one of them should be the MAIN port at a time. The
>>> role of the CPSW5G ports is decided based on what PHY port each of the CPSW5G
>>> ports connects to.
>>
>> OK, then instead of using bitmask and property being named "ti,enet-ctrl-qsgmii", why not
>> just say "ti,qsgmii-main-port" = <main_port_number>;
> 
> I plan to send patches for J721e device which has CPSW9G (8 external ports) MAC.
> CPSW9G can work with two sets of QSGMII interfaces (4 ports + 4 ports). Thus,
> using a bitmask for the QSGMII main port will help identify the QSGMII main port
> across both sets of QSGMII interfaces. The bitmask in case of J721e CPSW9G will
> consider the first 4 bits for the first interface's 4 ports and the next 4 bits
> for the second interface's 4 ports. In this manner, it will be possible to
> extend it for 8 port CPSW9G MAC as well, without having to add a new property
> for the second QSGMII interface.
> 
>>
>> Also do some sanity check when getting that property.
> 
> To ensure that multiple QSGMII ports are not declared as the main port, the
> "ti,enet-ctrl-qsgmii" property has been declared as an enum: [1,2,4,8]. If a

All I'm saying is that instead of bitmask please use port number to specify main port.
You can use minimum/maximum to limit the values.

Take care of limit checking per compatible and converting into bitmask in the driver.

> different value other than the value in enum were to be used, then "make
> dtbs_check" would raise an error. This will prevent configuring multiple QSGMII
> ports as the main port at once. Also, in the phy-gmii-sel driver, a default
> value of 1 is being assigned to the variable that will store the value
> corresponding to the ti,enet-ctrl-qsgmii property from the device tree, thereby
> treating the first CPSW5G port as the QSGMII main port by default.
> 
> Thanks,
> Siddharth.

cheers,
-roger

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

[Siddharth Vadapalli]

Hello Roger,

On 03/06/22 18:06, Roger Quadros wrote:
> Siddharth,
> 
> On 03/06/2022 13:49, Siddharth Vadapalli wrote:
>> Hello Roger,
>>
>> On 03/06/22 14:18, Roger Quadros wrote:
>>> Hi Siddharth,
>>>
>>> On 01/06/2022 14:27, Siddharth Vadapalli wrote:
>>>> Hello Roger,
>>>>
>>>> On 01/06/22 15:08, Roger Quadros wrote:
>>>>> Siddharth,
>>>>>
>>>>> On 01/06/2022 09:01, Siddharth Vadapalli wrote:
>>>>>> Hello Roger,
>>>>>>
>>>>>> On 31/05/22 17:15, Roger Quadros wrote:
>>>>>>> Hi Siddharth,
>>>>>>>
>>>>>>> On 31/05/2022 14:12, 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       |  5 ++++
>>>>>>>>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 24 ++++++++++++++++++-
>>>>>>>>  2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>>>>>>>> @@ -48,6 +48,11 @@ patternProperties:
>>>>>>>>      description:
>>>>>>>>        This is the SERDES lane control mux.
>>>>>>>>  
>>>>>>>> +  "phy@[0-9a-f]+$":
>>>>>>>> +    type: object
>>>>>>>> +    description:
>>>>>>>> +      This is the register to set phy mode through phy-gmii-sel driver.
>>>>>>>> +
>>>>>>>
>>>>>>> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all.
>>>>>>
>>>>>> The property has to be mentioned in order to pass: make dtbs_check.
>>>>>>
>>>>>>>
>>>>>>>>  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..7427758451e7 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>>>>>>> @@ -53,12 +53,21 @@ properties:
>>>>>>>>        - ti,am43xx-phy-gmii-sel
>>>>>>>>        - ti,dm814-phy-gmii-sel
>>>>>>>>        - ti,am654-phy-gmii-sel
>>>>>>>> +      - ti,j7200-cpsw5g-phy-gmii-sel
>>>>>>>
>>>>>>> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming.
>>>>>>
>>>>>> In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While
>>>>>> CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added
>>>>>> with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for
>>>>>> CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name
>>>>>> of the compatible.
>>>>>
>>>>> Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G)
>>>>> Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong.
>>>>
>>>> The PHY on J7200 is part of the Add-On Ethernet card. It is possible to connect
>>>> RGMII, QSGMII and SGMII PHY. The CPSW5G MAC supports all these modes. With the
>>>> current patch, I am adding just QSGMII mode as an extra mode, but in a future
>>>> patch, I will be adding SGMII also as an extra mode. For this reason, CPSW5G is
>>>> being mentioned in the compatible name, to differentiate supported modes for
>>>> CPSW2G and CPSW5G. Also, the phy-gmii-sel driver actually configures CPSW MAC
>>>> registers and not the PHY.
>>>
>>> phy-gmii-sel configures CTRL MMR register right? How does it configure CPSW MAC register?
>>>
>>> Anyways, I just looked at the TRM and there are in fact separate phy-gmii-sel (ENET_CTRL)
>>> registers for CPSW2g and CPSW5g. So they warrant for separate compatibles as they are
>>> not identical.
>>
>> By CPSW MAC registers being configured, I meant that the configuration being
>> done is for the MAC and not for the PHY. As per the TRM, for CPSW2G, the
>> CTRLMMR_MCU_ENET_CTRL register is configured and for CPSW5G, the
>> CTRLMMR_ENETx_CTRL registers are configured, with x ranging from 1 to 4
>> (corresponding to the 4 ports of CPSW5G). These registers configure the CPSW MAC
>> (CPSW2G/CPSW5G) and not the PHY. For this reason, I think that it would be
>> appropriate to use cpsw5g in the compatible name, to indicate which CTRLMMR
>> registers are being configured.
> 
> Yes, I already agreed that separate compatible is fine :).
> 
>>
>>>>
>>>>>
>>>>> You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode.
>>>>
>>>> Yes, I will add a check there too by using a different compatible in the CPSW
>>>> driver, but shouldn't the phy-gmii-sel driver also have a check to ensure that
>>>> it doesn't try configuring QSGMII mode for CPSW2G?
>>>
>>> Yes, additional check in phy-gmii-sel driver is fine.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>  
>>>>>>>>    reg:
>>>>>>>>      maxItems: 1
>>>>>>>>  
>>>>>>>>    '#phy-cells': true
>>>>>>>>  
>>>>>>>> +  ti,enet-ctrl-qsgmii:
>>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>>> +    description: |
>>>>>>>> +      Required only for QSGMII mode. Bitmask to select the port for
>>>>>>>> +      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>>>>>>>> +      ports automatically. Any of the 4 CPSW5G ports can act as the
>>>>>>>> +      main port with the rest of them being the QSGMII_SUB ports.
>>>>>>>> +
>>>>>>>
>>>>>>> This is weird way of doing things.
>>>>>>>
>>>>>>> The Ethernet controller driver already knows which mode the port is
>>>>>>> supposed to operate.
>>>>>>
>>>>>> From the ethernet driver perspective, there is no difference between the QSGMII
>>>>>> or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel
>>>>>> driver configures CPSW MAC registers differently depending on the mode being
>>>
>>> You mean the ENET_CTRL register in CTRL_MMR space?
>>
>> Yes I am referring to the CTRLMMR_ENETx_CTRL registers as per the J7200 TRM,
>> corresponding to the CPSW5G MAC.
>>
>>>
>>>>>> QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to
>>>>>> identify the QSGMII main port and the rest are configured in CPSW MAC as
>>>>>> QSGMII-SUB ports.
>>>>>>
>>>>>>>
>>>>>>> e.g.
>>>>>>> +&cpsw0_port1 {
>>>>>>> +	phy-handle = <&cpsw5g_phy0>;
>>>>>>> +	phy-mode = "qsgmii";
>>>>>>> +	mac-address = [00 00 00 00 00 00];
>>>>>>> +	phys = <&cpsw0_phy_gmii_sel 1>;
>>>>>>> +};
>>>>>>> +
>>>>>>> +&cpsw0_port2 {
>>>>>>> +	phy-handle = <&cpsw5g_phy1>;
>>>>>>> +	phy-mode = "qsgmii-sub";
>>>>>>> +	mac-address = [00 00 00 00 00 00];
>>>>>>> +	phys = <&cpsw0_phy_gmii_sel 2>;
>>>>>>>
>>>>>>> And it can convey the mode to the PHY driver via phy_ops->set_mode.
>>>>>>> So you should be depending on that instead of adding this new property.
>>>>>>
>>>>>> QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with
>>>>>> the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB
>>>>>> has to be introduced to the kernel. Additionally, all existing phy drivers will
>>>>>> have to be updated to recognize the new phy mode.
>>>>>>
>>>>>> Since the QSGMII-SUB mode is TI specific, it was decided that it would be better
>>>>>> to add a new property in TI specific files for identifying the QSGMII main port
>>>>>> and treating the rest as QSGMII-SUB ports.
>>>>>
>>>>> Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN?
>>>>> Can all ports be SUB or there has to be at least one MAIN?
>>>>
>>>> All 4 ports in CPSW5G have the capability to be the MAIN port, with the only
>>>> restriction being that only one of them should be the MAIN port at a time. The
>>>> role of the CPSW5G ports is decided based on what PHY port each of the CPSW5G
>>>> ports connects to.
>>>
>>> OK, then instead of using bitmask and property being named "ti,enet-ctrl-qsgmii", why not
>>> just say "ti,qsgmii-main-port" = <main_port_number>;
>>
>> I plan to send patches for J721e device which has CPSW9G (8 external ports) MAC.
>> CPSW9G can work with two sets of QSGMII interfaces (4 ports + 4 ports). Thus,
>> using a bitmask for the QSGMII main port will help identify the QSGMII main port
>> across both sets of QSGMII interfaces. The bitmask in case of J721e CPSW9G will
>> consider the first 4 bits for the first interface's 4 ports and the next 4 bits
>> for the second interface's 4 ports. In this manner, it will be possible to
>> extend it for 8 port CPSW9G MAC as well, without having to add a new property
>> for the second QSGMII interface.
>>
>>>
>>> Also do some sanity check when getting that property.
>>
>> To ensure that multiple QSGMII ports are not declared as the main port, the
>> "ti,enet-ctrl-qsgmii" property has been declared as an enum: [1,2,4,8]. If a
> 
> All I'm saying is that instead of bitmask please use port number to specify main port.
> You can use minimum/maximum to limit the values.
> 
> Take care of limit checking per compatible and converting into bitmask in the driver.

The current series of patches is for J7200 device which supports one QSGMII
interface. However, I plan to post patches for another device (J721e) which has
8 external ports and therefore, can be configured as two sets of QSGMII
interfaces. To identify the two main ports across the two QSGMII interfaces, a
single port number will not be sufficient. Hence, a bitmask has been used, to
avoid adding a new property for the second QSGMII interface's main port, when I
post patches for J721e.

Thanks,
Siddharth.