[PATCH v5 0/3] Add support for QSGMII mode

[PATCH v5 0/3] 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:

v4 -> v5:
1. Undo cleanup changes in the example in:
   Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml, moving it
   to a separate patch in the same series. The cleanup patch is the first
   patch in this series, with the other patches from previous versions
   following it in the same order.
2. Update $ref to "/schemas/phy/ti,phy-gmii-sel.yaml#" in:
   Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml.
3. Update description for the "phy@[0-9a-f]+$" pattern property.
4. Add blank lines in between IF/THEN statements in:
   Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml.
5. Update the IF/THEN statement corresponding to the property
   "ti,qsgmii-main-ports", disallowing the property for all compatibles
   except "ti,j7200-cpsw5g-phy-gmii-sel" in:
   Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml.
6. Move "maxItems: 1" to the top where "ti,qsgmii-main-ports" property is
   first defined, in:
   Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml.

v3 -> v4:
1. Update $ref to /schemas/phy/phy-provider.yaml in
   Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml.
2. Update commit message for the "phy: ti: gmii-sel: Add support for
   CPSW5G GMII SEL in J7200" patch, describing the reason for defining the
   property "ti,qsgmii-main-ports" as an array.
3. Add a check in drivers/phy/ti/phy-gmii-sel.c to ensure that the value
   of the variable "main_ports" is within bounds. If the property
   "ti,qsgmii-main-ports" is either not mentioned in the devicetree or the
   value of the property is out of bounds, in both these cases,
   "main_ports" defaults to 1.
4. Use the function "of_property_read_u32()" instead of the function
   "of_property_read_u32_array()" in drivers/phy/ti/phy-gmii-sel.c.

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.

v4: https://lore.kernel.org/r/20220901085506.138633-1-s-vadapalli@ti.com/
v3: https://lore.kernel.org/r/20220822065631.27933-1-s-vadapalli@ti.com/
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 (3):
  dt-bindings: phy: ti: phy-gmii-sel: Cleanup example
  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         | 27 ++++++++++-
 drivers/phy/ti/phy-gmii-sel.c                 | 47 +++++++++++++++++--
 3 files changed, 76 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v5 1/3] dt-bindings: phy: ti: phy-gmii-sel: Cleanup example

[Siddharth Vadapalli]

Change node name in example from "phy-gmii-sel" to "phy", following the
device-tree convention of using generic node names.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
index ff8a6d9eb153..016a37db1ea1 100644
--- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
@@ -97,7 +97,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 v5 1/3] dt-bindings: phy: ti: phy-gmii-sel: Cleanup example

[Krzysztof Kozlowski]

On 12/09/2022 10:56, Siddharth Vadapalli wrote:
> Change node name in example from "phy-gmii-sel" to "phy", following the
> device-tree convention of using generic node names.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

[PATCH v5 2/3] 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         | 25 +++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
index 1aeac43cad92..873ee0c0973f 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: /schemas/phy/ti,phy-gmii-sel.yaml#
+    description:
+      The phy node corresponding to the ethernet MAC.
+
 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 016a37db1ea1..da7cac537e15 100644
--- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
@@ -53,12 +53,25 @@ 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.
+    maxItems: 1
+    items:
+      minimum: 1
+      maximum: 4
+
 allOf:
   - if:
       properties:
@@ -73,6 +86,18 @@ allOf:
         '#phy-cells':
           const: 1
           description: CPSW port number (starting from 1)
+
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - ti,j7200-cpsw5g-phy-gmii-sel
+    then:
+      properties:
+        ti,qsgmii-main-ports: false
+
   - if:
       properties:
         compatible:
-- 
2.25.1

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

[Krzysztof Kozlowski]

On 12/09/2022 10:56, Siddharth Vadapalli wrote:

>  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 016a37db1ea1..da7cac537e15 100644
> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> @@ -53,12 +53,25 @@ 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

Not really an array...

> +      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
> +      ports automatically. Any one of the 4 CPSW5G ports can act as the
> +      main port with the rest of them being the QSGMII_SUB ports.
> +    maxItems: 1


You say it is an array, but you have here just one item, so it is just
uint32. Do you expect it to grow? If so, when? Why it cannot grow now?



Best regards,
Krzysztof

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

[Siddharth Vadapalli]

Hello Krzysztof,

On 13/09/22 14:57, Krzysztof Kozlowski wrote:
> On 12/09/2022 10:56, Siddharth Vadapalli wrote:
> 
>>  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 016a37db1ea1..da7cac537e15 100644
>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> @@ -53,12 +53,25 @@ 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
> 
> Not really an array...
> 
>> +      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>> +      ports automatically. Any one of the 4 CPSW5G ports can act as the
>> +      main port with the rest of them being the QSGMII_SUB ports.
>> +    maxItems: 1
> 
> 
> You say it is an array, but you have here just one item, so it is just
> uint32. Do you expect it to grow? If so, when? Why it cannot grow now?

Thank you for reviewing the patch.

I have defined it as an array because I plan to reuse this property for
other TI devices like J721e which supports up to two QSGMII main ports.
J7200 on the other hand can have at most one QSGMII main port, which is
why I have restricted the array size to one element as of this series.
In the upcoming patches that I will be posting for J721e, I will be
changing the maxItems to 2 for J721e's compatible while it will continue
to remain 1 for J7200's compatible. This is the reason for defining the
property as an array.

Regards,
Siddharth.

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

[Krzysztof Kozlowski]

On 13/09/2022 11:45, Siddharth Vadapalli wrote:
> Hello Krzysztof,
> 
> On 13/09/22 14:57, Krzysztof Kozlowski wrote:
>> On 12/09/2022 10:56, Siddharth Vadapalli wrote:
>>
>>>  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 016a37db1ea1..da7cac537e15 100644
>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>>> @@ -53,12 +53,25 @@ 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
>>
>> Not really an array...
>>
>>> +      QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>>> +      ports automatically. Any one of the 4 CPSW5G ports can act as the
>>> +      main port with the rest of them being the QSGMII_SUB ports.
>>> +    maxItems: 1
>>
>>
>> You say it is an array, but you have here just one item, so it is just
>> uint32. Do you expect it to grow? If so, when? Why it cannot grow now?
> 
> Thank you for reviewing the patch.
> 
> I have defined it as an array because I plan to reuse this property for
> other TI devices like J721e which supports up to two QSGMII main ports.
> J7200 on the other hand can have at most one QSGMII main port, which is
> why I have restricted the array size to one element as of this series.
> In the upcoming patches that I will be posting for J721e, I will be
> changing the maxItems to 2 for J721e's compatible while it will continue
> to remain 1 for J7200's compatible. This is the reason for defining the
> property as an array.

I have an impression that I asked this and you already replied... so
apologies for asking again. :)


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

[PATCH v5 3/3] 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. The property "ti,qsgmii-main-ports" is used to configure the
CTRLMMR_ENETx_CTRL register.

Depending on the device, it is possible for more than one QSGMII main port
to exist. Thus, the property "ti,qsgmii-main-ports" is defined as an array
of values in order to reuse the property for other devices.

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

diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
index d0ab69750c6b..0bcfd6d96b4d 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,15 @@ 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(node, "ti,qsgmii-main-ports", &main_ports);
+	/*
+	 * Ensure that main_ports is within bounds. If the property
+	 * ti,qsgmii-main-ports is not mentioned, or the value mentioned
+	 * is out of bounds, default to 1.
+	 */
+	if (main_ports < 1 || main_ports > 4)
+		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 v5 0/3] Add support for QSGMII mode

[Vinod Koul]

On 12-09-22, 14:26, Siddharth Vadapalli wrote:
> Add compatible for J7200 CPSW5G.
> 
> Add support for QSGMII mode in phy-gmii-sel driver for CPSW5G in J7200.

Applied, thanks

-- 
~Vinod