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

[PATCH v4 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:

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.

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 (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                 | 47 +++++++++++++++++--
 3 files changed, 79 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v4 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 1aeac43cad92..802374e7645f 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/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
+    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

[PATCH v4 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. 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 v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200

[Krzysztof Kozlowski]

On 01/09/2022 11:55, 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 1aeac43cad92..802374e7645f 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/phy-provider.yaml

You need instead ref to specific device bindings/schema. Probably to
/schemas/phy/ti,phy-gmii-sel.yaml#

This was entirely different in v3, so your change is very confusing.

> +    description:
> +      This is the register to set phy mode through phy-gmii-sel driver.

I don't understand the description. Please focus on the hardware not
some drivers - what is here? Phy for something?

> +
>  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)

Blank line


> +  - 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

It does not really make sense to limit items here, in the context of
this patch. You got a comment for it already. Your patch should make
sense on its own.

> +    else:
> +      properties:
> +        ti,qsgmii-main-ports: false

Blank line

>    - if:
>        properties:
>          compatible:
> @@ -97,7 +125,7 @@ additionalProperties: false
>  
>  examples:
>    - |
> -    phy_gmii_sel: phy-gmii-sel@650 {
> +    phy_gmii_sel: phy@650 {

Split cleanup into separate patch.

>          compatible = "ti,am3352-phy-gmii-sel";
>          reg = <0x650 0x4>;
>          #phy-cells = <2>;


Best regards,
Krzysztof

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

[Siddharth Vadapalli]

Hello Krzysztof,

On 01/09/22 20:51, Krzysztof Kozlowski wrote:
> On 01/09/2022 11:55, 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 1aeac43cad92..802374e7645f 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/phy-provider.yaml
> 
> You need instead ref to specific device bindings/schema. Probably to
> /schemas/phy/ti,phy-gmii-sel.yaml#

Thank you for the clarification. I will update $ref to
"/schemas/phy/ti,phy-gmii-sel.yaml#" in the v5 series.

> 
> This was entirely different in v3, so your change is very confusing.

I had misunderstood Rob's comment in the v3 patch. I had initially
provided the relative path to the bindings file ti,phy-gmii-sel.yaml in
the v3 patch. When Rob commented "/schemas/phy/..", I misunderstood and
thought that I had to point $ref to a generic phy-provider schema
present in the dt-schema repo and thus, in this v4 patch, I had updated
$ref accordingly.

> 
>> +    description:
>> +      This is the register to set phy mode through phy-gmii-sel driver.
> 
> I don't understand the description. Please focus on the hardware not
> some drivers - what is here? Phy for something?

I will fix the description, updating it to the following:
"Address of the CTRLMMR_ENETx_CTRL registers which are used to configure
the phy-mode of the CPSW MAC ports."

Please let me know if the above description is fine.

> 
>> +
>>  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)
> 
> Blank line

I will fix this in the v5 series.

> 
> 
>> +  - 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
> 
> It does not really make sense to limit items here, in the context of
> this patch. You got a comment for it already. Your patch should make
> sense on its own.

I had defined the property as an array because there are more than one
QSGMII main ports for other devices for which I will be posting the
patches. I had planned to reuse this property, with "maxItems: 2" in the
future patches for other compatibles. However, as suggested by you, I
will change the property to a uint32 instead of uint32-array in this
series. Later, in my future patches for other devices, I will change it
back to a uint32-array when I reuse the property.

> 
>> +    else:
>> +      properties:
>> +        ti,qsgmii-main-ports: false
> 
> Blank line

I will fix this in the v5 series.

> 
>>    - if:
>>        properties:
>>          compatible:
>> @@ -97,7 +125,7 @@ additionalProperties: false
>>  
>>  examples:
>>    - |
>> -    phy_gmii_sel: phy-gmii-sel@650 {
>> +    phy_gmii_sel: phy@650 {
> 
> Split cleanup into separate patch.

I will do so. Thank you for reviewing the patch.

Regards,
Siddharth.

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

[Krzysztof Kozlowski]

On 02/09/2022 08:09, Siddharth Vadapalli wrote:
> Hello Krzysztof,
> 
> On 01/09/22 20:51, Krzysztof Kozlowski wrote:
>> On 01/09/2022 11:55, 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 1aeac43cad92..802374e7645f 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/phy-provider.yaml
>>
>> You need instead ref to specific device bindings/schema. Probably to
>> /schemas/phy/ti,phy-gmii-sel.yaml#
> 
> Thank you for the clarification. I will update $ref to
> "/schemas/phy/ti,phy-gmii-sel.yaml#" in the v5 series.
> 
>>
>> This was entirely different in v3, so your change is very confusing.
> 
> I had misunderstood Rob's comment in the v3 patch. I had initially
> provided the relative path to the bindings file ti,phy-gmii-sel.yaml in
> the v3 patch. When Rob commented "/schemas/phy/..", I misunderstood and
> thought that I had to point $ref to a generic phy-provider schema
> present in the dt-schema repo and thus, in this v4 patch, I had updated
> $ref accordingly.
> 
>>
>>> +    description:
>>> +      This is the register to set phy mode through phy-gmii-sel driver.
>>
>> I don't understand the description. Please focus on the hardware not
>> some drivers - what is here? Phy for something?
> 
> I will fix the description, updating it to the following:
> "Address of the CTRLMMR_ENETx_CTRL registers which are used to configure
> the phy-mode of the CPSW MAC ports."
> 
> Please let me know if the above description is fine.

Hm, but that's a phy node, not address of register... Isn't this a phy
node representing the phy of the CPSW MAC ports?

> 
>>
>>> +
>>>  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)
>>
>> Blank line
> 
> I will fix this in the v5 series.
> 
>>
>>
>>> +  - 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
>>
>> It does not really make sense to limit items here, in the context of
>> this patch. You got a comment for it already. Your patch should make
>> sense on its own.
> 
> I had defined the property as an array because there are more than one
> QSGMII main ports for other devices for which I will be posting the
> patches. I had planned to reuse this property, with "maxItems: 2" in the
> future patches for other compatibles. However, as suggested by you, I
> will change the property to a uint32 instead of uint32-array in this
> series. Later, in my future patches for other devices, I will change it
> back to a uint32-array when I reuse the property.

Wait, no. You should not change the property. This should be
uint32-array, because you will extend it soon, just maxItems must be
defined in top-level place.

Best regards,
Krzysztof

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

[Siddharth Vadapalli]

Hello Krzysztof,

On 05/09/22 18:39, Krzysztof Kozlowski wrote:
> On 02/09/2022 08:09, Siddharth Vadapalli wrote:
>> Hello Krzysztof,
>>
>> On 01/09/22 20:51, Krzysztof Kozlowski wrote:
>>> On 01/09/2022 11:55, 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 1aeac43cad92..802374e7645f 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/phy-provider.yaml
>>>
>>> You need instead ref to specific device bindings/schema. Probably to
>>> /schemas/phy/ti,phy-gmii-sel.yaml#
>>
>> Thank you for the clarification. I will update $ref to
>> "/schemas/phy/ti,phy-gmii-sel.yaml#" in the v5 series.
>>
>>>
>>> This was entirely different in v3, so your change is very confusing.
>>
>> I had misunderstood Rob's comment in the v3 patch. I had initially
>> provided the relative path to the bindings file ti,phy-gmii-sel.yaml in
>> the v3 patch. When Rob commented "/schemas/phy/..", I misunderstood and
>> thought that I had to point $ref to a generic phy-provider schema
>> present in the dt-schema repo and thus, in this v4 patch, I had updated
>> $ref accordingly.
>>
>>>
>>>> +    description:
>>>> +      This is the register to set phy mode through phy-gmii-sel driver.
>>>
>>> I don't understand the description. Please focus on the hardware not
>>> some drivers - what is here? Phy for something?
>>
>> I will fix the description, updating it to the following:
>> "Address of the CTRLMMR_ENETx_CTRL registers which are used to configure
>> the phy-mode of the CPSW MAC ports."
>>
>> Please let me know if the above description is fine.
> 
> Hm, but that's a phy node, not address of register... Isn't this a phy
> node representing the phy of the CPSW MAC ports?

Despite it being a phy node, the phy-gmii-sel driver actually uses this
node to obtain the address of the CTRLMMR_ENETx_CTRL registers which
correspond to the CPSW MAC configuration and are therefore unrelated to
the PHY. Please let me know if my suggested description would be fine.

> 
>>
>>>
>>>> +
>>>>  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)
>>>
>>> Blank line
>>
>> I will fix this in the v5 series.
>>
>>>
>>>
>>>> +  - 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
>>>
>>> It does not really make sense to limit items here, in the context of
>>> this patch. You got a comment for it already. Your patch should make
>>> sense on its own.
>>
>> I had defined the property as an array because there are more than one
>> QSGMII main ports for other devices for which I will be posting the
>> patches. I had planned to reuse this property, with "maxItems: 2" in the
>> future patches for other compatibles. However, as suggested by you, I
>> will change the property to a uint32 instead of uint32-array in this
>> series. Later, in my future patches for other devices, I will change it
>> back to a uint32-array when I reuse the property.
> 
> Wait, no. You should not change the property. This should be
> uint32-array, because you will extend it soon, just maxItems must be
> defined in top-level place.

Thank you for clarifying. I will move "maxItems: 1" to the top where
"ti,qsgmii-main-ports" property is first defined, and continue using the
property as a uint32-array.

Regards,
Siddharth.

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

[Krzysztof Kozlowski]

On 06/09/2022 07:02, Siddharth Vadapalli wrote:
>>>
>>> Please let me know if the above description is fine.
>>
>> Hm, but that's a phy node, not address of register... Isn't this a phy
>> node representing the phy of the CPSW MAC ports?
> 
> Despite it being a phy node, the phy-gmii-sel driver actually uses this
> node to obtain the address of the CTRLMMR_ENETx_CTRL registers which
> correspond to the CPSW MAC configuration and are therefore unrelated to
> the PHY. Please let me know if my suggested description would be fine.

Either I miss some more pieces or this is wrong design. The phy node
should not be used to pass some addresses somewhere. It is used to
define a device which will be instantiated (as parent is simple-mfd). If
you use it only to obtain some address, not to describe child device,
then this is wrong property type.

Best regards,
Krzysztof

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

[Siddharth Vadapalli]

Hello Krzysztof,

On 06/09/22 12:33, Krzysztof Kozlowski wrote:
> On 06/09/2022 07:02, Siddharth Vadapalli wrote:
>>>>
>>>> Please let me know if the above description is fine.
>>>
>>> Hm, but that's a phy node, not address of register... Isn't this a phy
>>> node representing the phy of the CPSW MAC ports?
>>
>> Despite it being a phy node, the phy-gmii-sel driver actually uses this
>> node to obtain the address of the CTRLMMR_ENETx_CTRL registers which
>> correspond to the CPSW MAC configuration and are therefore unrelated to
>> the PHY. Please let me know if my suggested description would be fine.
> 
> Either I miss some more pieces or this is wrong design. The phy node
> should not be used to pass some addresses somewhere. It is used to
> define a device which will be instantiated (as parent is simple-mfd). If
> you use it only to obtain some address, not to describe child device,
> then this is wrong property type.

Sorry for describing it incompletely, and at some places incorrectly.
Yes, you were right when you initially mentioned that the phy node
corresponds to the phy of the ethernet MAC ports. I had incorrectly
understood the term "phy" there as the external Layer-1 ethernet phy.The
phy node corresponds to the phy used by the ethernet MAC, based on the
phy-mode configured. The am65-cpsw-nuss driver which is responsible for
ethernet MAC, requires the ethernet MAC's phy to be configured by the
phy-gmii-sel driver. Thus, the phy node corresponds to an actual phy
(ethernet MAC's PHY).

I plan on updating the description for the phy pattern property to the
following:
"The phy node corresponding to the ethernet MAC."

Regards,
Siddharth.