[PATCH net-next v6 0/3] Add support for QSGMII mode for J721e CPSW9G to am65-cpsw driver

[PATCH net-next v6 0/3] Add support for QSGMII mode for J721e CPSW9G to am65-cpsw driver

[Siddharth Vadapalli]

Add compatible to am65-cpsw driver for J721e CPSW9G, which contains 8
external ports and 1 internal host port.

Add support to power on and power off the SERDES PHY which is used by the
CPSW MAC.

=========
Changelog
=========
v5 -> v6:
1. Add member "serdes_phy" in struct "am65_cpsw_slave_data" to store the
   SERDES PHY for each port, if present. This is done to cache the SERDES
   PHY beforehand, without depending on devm_of_phy_get().
2. Rebase series on net-next tree.

v4 -> v5:
1. Update subject of all patches in the series to "PATCH net-next".
2. Rebase series on net-next tree.

v3 -> v4:
1. Fix subject of patch-1/3, updating it to:
   "dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G support"
   and collect Reviewed-by tag.
2. Rebase series on linux-next tree tagged: next-20221107.

v2 -> v3:
1. Run 'make DT_CHECKER_FLAGS=-m dt_binding_check' and fix errors and
   warnings corresponding to the patch for:
   Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
   with the latest dt-schema and yamllint.

v1 -> v2:
1. Drop all patches corresponding to SGMII mode. This is done since I do
   not have a method to test SGMII in the standard mode which uses an
   SGMII PHY. The previous series used SGMII in a fixed-link mode,
   bypassing the SGMII PHY. I will post the SGMII patches in a future
   series after testing them.
2. Drop all patches corresponding to fixed-link in the am65-cpsw driver.
   This is done since PHYLINK takes care of fixed-link automatically and
   there is no need to deal with fixed-link in a custom fashion.
3. Fix indentation errors in k3-am65-cpsw-nuss.yaml.
4. Remove the stale code which tries to power on and power off the CPSW
   MAC's phy, since the CPSW MAC's phy driver does not support it.
5. Rename the function "am65_cpsw_init_phy()" to
   "am65_cpsw_init_serdes_phy()", to indicate that the phy corresponds to
   the SERDES.
6. Invoke "am65_cpsw_disable_serdes_phy()" as a part of the cleanup that
   is associated with the "am65_cpsw_nuss_remove()" function.

v5:
https://lore.kernel.org/r/20221109042203.375042-1-s-vadapalli@ti.com/
v4:
https://lore.kernel.org/r/20221108080606.124596-1-s-vadapalli@ti.com/
v3:
https://lore.kernel.org/r/20221026090957.180592-1-s-vadapalli@ti.com/
v2:
https://lore.kernel.org/r/20221018085810.151327-1-s-vadapalli@ti.com/
v1:
https://lore.kernel.org/r/20220914095053.189851-1-s-vadapalli@ti.com/

Siddharth Vadapalli (3):
  dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G support
  net: ethernet: ti: am65-cpsw: Enable QSGMII mode for J721e CPSW9G
  net: ethernet: ti: am65-cpsw: Add support for SERDES configuration

 .../bindings/net/ti,k3-am654-cpsw-nuss.yaml   | 33 +++++++-
 drivers/net/ethernet/ti/am65-cpsw-nuss.c      | 76 +++++++++++++++++++
 drivers/net/ethernet/ti/am65-cpsw-nuss.h      |  1 +
 3 files changed, 106 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH net-next v6 1/3] dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G support

[Siddharth Vadapalli]

Update bindings for TI K3 J721e SoC which contains 9 ports (8 external
ports) CPSW9G module and add compatible for it.

Changes made:
    - Add new compatible ti,j721e-cpswxg-nuss for CPSW9G.
    - Extend pattern properties for new compatible.
    - Change maximum number of CPSW ports to 8 for new compatible.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/net/ti,k3-am654-cpsw-nuss.yaml   | 33 ++++++++++++++++---
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
index 821974815dec..900063411a20 100644
--- a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
+++ b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
@@ -57,6 +57,7 @@ properties:
       - ti,am654-cpsw-nuss
       - ti,j7200-cpswxg-nuss
       - ti,j721e-cpsw-nuss
+      - ti,j721e-cpswxg-nuss
       - ti,am642-cpsw-nuss
 
   reg:
@@ -111,7 +112,7 @@ properties:
         const: 0
 
     patternProperties:
-      "^port@[1-4]$":
+      "^port@[1-8]$":
         type: object
         description: CPSWxG NUSS external ports
 
@@ -121,7 +122,7 @@ properties:
         properties:
           reg:
             minimum: 1
-            maximum: 4
+            maximum: 8
             description: CPSW port number
 
           phys:
@@ -186,12 +187,36 @@ allOf:
         properties:
           compatible:
             contains:
-              const: ti,j7200-cpswxg-nuss
+              const: ti,j721e-cpswxg-nuss
     then:
       properties:
         ethernet-ports:
           patternProperties:
-            "^port@[3-4]$": false
+            "^port@[5-8]$": false
+            "^port@[1-4]$":
+              properties:
+                reg:
+                  minimum: 1
+                  maximum: 4
+
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - ti,j721e-cpswxg-nuss
+                - ti,j7200-cpswxg-nuss
+    then:
+      properties:
+        ethernet-ports:
+          patternProperties:
+            "^port@[3-8]$": false
+            "^port@[1-2]$":
+              properties:
+                reg:
+                  minimum: 1
+                  maximum: 2
 
 additionalProperties: false
 
-- 
2.25.1

Re: [PATCH net-next v6 1/3] dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G support

[Geert Uytterhoeven]

Hi Siddharth,

On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
> Update bindings for TI K3 J721e SoC which contains 9 ports (8 external
> ports) CPSW9G module and add compatible for it.
>
> Changes made:
>     - Add new compatible ti,j721e-cpswxg-nuss for CPSW9G.
>     - Extend pattern properties for new compatible.
>     - Change maximum number of CPSW ports to 8 for new compatible.
>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks for your patch, which is now commit c85b53e32c8ecfe6
("dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G
support") in net-next.

You forgot to document the presence of the new optional
"serdes-phy" PHY.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH net-next v6 1/3] dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G support

[Krzysztof Kozlowski]

On 17/01/2023 14:45, Geert Uytterhoeven wrote:
> Hi Siddharth,
> 
> On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
>> Update bindings for TI K3 J721e SoC which contains 9 ports (8 external
>> ports) CPSW9G module and add compatible for it.
>>
>> Changes made:
>>     - Add new compatible ti,j721e-cpswxg-nuss for CPSW9G.
>>     - Extend pattern properties for new compatible.
>>     - Change maximum number of CPSW ports to 8 for new compatible.
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> Thanks for your patch, which is now commit c85b53e32c8ecfe6
> ("dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G
> support") in net-next.
> 
> You forgot to document the presence of the new optional
> "serdes-phy" PHY.

I think we should start rejecting most of bindings without DTS, because
submitters really like to forget to make complete bindings. Having a DTS
with such undocumented property gives a bit bigger chance it will get an
attention. :(

Best regards,
Krzysztof

Re: [PATCH net-next v6 1/3] dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G support

[Raghavendra, Vignesh]

On 1/17/2023 10:47 PM, Krzysztof Kozlowski wrote:
> On 17/01/2023 14:45, Geert Uytterhoeven wrote:
>> Hi Siddharth,
>>
>> On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
>>> Update bindings for TI K3 J721e SoC which contains 9 ports (8 external
>>> ports) CPSW9G module and add compatible for it.
>>>
>>> Changes made:
>>>     - Add new compatible ti,j721e-cpswxg-nuss for CPSW9G.
>>>     - Extend pattern properties for new compatible.
>>>     - Change maximum number of CPSW ports to 8 for new compatible.
>>>
>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>
>> Thanks for your patch, which is now commit c85b53e32c8ecfe6
>> ("dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G
>> support") in net-next.
>>
>> You forgot to document the presence of the new optional
>> "serdes-phy" PHY.
> 
> I think we should start rejecting most of bindings without DTS, because
> submitters really like to forget to make complete bindings. Having a DTS
> with such undocumented property gives a bit bigger chance it will get an
> attention. :(
> 

Agree, bindings should have been better tested against real DTS.

But for reviewers, this been a bit of chicken-egg problem. Bindings and
driver changes have to go in first and via "subsystem" trees while DTS
patches have to go via "arch" tree. So, they get posted separately.

One may not see DTS patches (and thus user of the bindings) until
bindings reach Torvalds' tree. So, user of bindings will only appear in
the next kernel release cycle (at which time they do get flagged due to
failing make dtbs_check but its bit late). Wondering how others are
managing the same ?


Regards
Vignesh

[PATCH net-next v6 2/3] net: ethernet: ti: am65-cpsw: Enable QSGMII mode for J721e CPSW9G

[Siddharth Vadapalli]

CPSW9G in J721e supports additional modes like QSGMII.
Add new compatible for J721e in am65-cpsw driver.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 9decb0c7961b..06912363d5d5 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2684,11 +2684,19 @@ static const struct am65_cpsw_pdata j7200_cpswxg_pdata = {
 	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
 };
 
+static const struct am65_cpsw_pdata j721e_cpswxg_pdata = {
+	.quirks = 0,
+	.ale_dev_id = "am64-cpswxg",
+	.fdqring_mode = K3_RINGACC_RING_MODE_MESSAGE,
+	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
+};
+
 static const struct of_device_id am65_cpsw_nuss_of_mtable[] = {
 	{ .compatible = "ti,am654-cpsw-nuss", .data = &am65x_sr1_0},
 	{ .compatible = "ti,j721e-cpsw-nuss", .data = &j721e_pdata},
 	{ .compatible = "ti,am642-cpsw-nuss", .data = &am64x_cpswxg_pdata},
 	{ .compatible = "ti,j7200-cpswxg-nuss", .data = &j7200_cpswxg_pdata},
+	{ .compatible = "ti,j721e-cpswxg-nuss", .data = &j721e_cpswxg_pdata},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, am65_cpsw_nuss_of_mtable);
-- 
2.25.1

[PATCH net-next v6 3/3] net: ethernet: ti: am65-cpsw: Add support for SERDES configuration

[Siddharth Vadapalli]

Use PHY framework APIs to initialize the SERDES PHY connected to CPSW MAC.

Define the functions am65_cpsw_disable_phy(), am65_cpsw_enable_phy(),
am65_cpsw_disable_serdes_phy() and am65_cpsw_enable_serdes_phy().

Add new member "serdes_phy" to struct "am65_cpsw_slave_data" to store the
SERDES PHY for each port, if it exists. Use it later while disabling the
SERDES PHY for each port.

Power on and initialize the SerDes PHY in am65_cpsw_nuss_init_slave_ports()
by invoking am65_cpsw_enable_serdes_phy().

Power off the SerDes PHY in am65_cpsw_nuss_remove() by invoking
am65_cpsw_disable_serdes_phy().

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 68 ++++++++++++++++++++++++
 drivers/net/ethernet/ti/am65-cpsw-nuss.h |  1 +
 2 files changed, 69 insertions(+)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 06912363d5d5..1bd11166dc28 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -1416,6 +1416,68 @@ static const struct net_device_ops am65_cpsw_nuss_netdev_ops = {
 	.ndo_setup_tc           = am65_cpsw_qos_ndo_setup_tc,
 };
 
+static void am65_cpsw_disable_phy(struct phy *phy)
+{
+	phy_power_off(phy);
+	phy_exit(phy);
+}
+
+static int am65_cpsw_enable_phy(struct phy *phy)
+{
+	int ret;
+
+	ret = phy_init(phy);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_power_on(phy);
+	if (ret < 0) {
+		phy_exit(phy);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void am65_cpsw_disable_serdes_phy(struct am65_cpsw_common *common)
+{
+	struct am65_cpsw_port *port;
+	struct phy *phy;
+	int i;
+
+	for (i = 0; i < common->port_num; i++) {
+		port = &common->ports[i];
+		phy = port->slave.serdes_phy;
+		if (phy)
+			am65_cpsw_disable_phy(phy);
+	}
+}
+
+static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *port_np,
+				     struct am65_cpsw_port *port)
+{
+	const char *name = "serdes-phy";
+	struct phy *phy;
+	int ret;
+
+	phy = devm_of_phy_get(dev, port_np, name);
+	if (PTR_ERR(phy) == -ENODEV)
+		return 0;
+
+	/* Serdes PHY exists. Store it. */
+	port->slave.serdes_phy = phy;
+
+	ret =  am65_cpsw_enable_phy(phy);
+	if (ret < 0)
+		goto err_phy;
+
+	return 0;
+
+err_phy:
+	devm_phy_put(dev, phy);
+	return ret;
+}
+
 static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned int mode,
 				      const struct phylink_link_state *state)
 {
@@ -1959,6 +2021,11 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
 			goto of_node_put;
 		}
 
+		/* Initialize the Serdes PHY for the port */
+		ret = am65_cpsw_init_serdes_phy(dev, port_np, port);
+		if (ret)
+			return ret;
+
 		port->slave.mac_only =
 				of_property_read_bool(port_np, "ti,mac-only");
 
@@ -2878,6 +2945,7 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev)
 	 */
 	am65_cpsw_nuss_cleanup_ndev(common);
 	am65_cpsw_nuss_phylink_cleanup(common);
+	am65_cpsw_disable_serdes_phy(common);
 
 	of_platform_device_destroy(common->mdio_dev, NULL);
 
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
index 4b75620f8d28..ed26768a6e51 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
@@ -32,6 +32,7 @@ struct am65_cpsw_slave_data {
 	struct device_node		*phy_node;
 	phy_interface_t			phy_if;
 	struct phy			*ifphy;
+	struct phy			*serdes_phy;
 	bool				rx_pause;
 	bool				tx_pause;
 	u8				mac_addr[ETH_ALEN];
-- 
2.25.1

Re: [PATCH net-next v6 3/3] net: ethernet: ti: am65-cpsw: Add support for SERDES configuration

[Geert Uytterhoeven]

Hi Siddharth,

On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
> Use PHY framework APIs to initialize the SERDES PHY connected to CPSW MAC.
>
> Define the functions am65_cpsw_disable_phy(), am65_cpsw_enable_phy(),
> am65_cpsw_disable_serdes_phy() and am65_cpsw_enable_serdes_phy().
>
> Add new member "serdes_phy" to struct "am65_cpsw_slave_data" to store the
> SERDES PHY for each port, if it exists. Use it later while disabling the
> SERDES PHY for each port.
>
> Power on and initialize the SerDes PHY in am65_cpsw_nuss_init_slave_ports()
> by invoking am65_cpsw_enable_serdes_phy().
>
> Power off the SerDes PHY in am65_cpsw_nuss_remove() by invoking
> am65_cpsw_disable_serdes_phy().
>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Thanks for your patch, which is now commit dab2b265dd23ef8f ("net:
ethernet: ti: am65-cpsw: Add support for SERDES configuration")
in net-next.

> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -1416,6 +1416,68 @@ static const struct net_device_ops am65_cpsw_nuss_netdev_ops = {
>         .ndo_setup_tc           = am65_cpsw_qos_ndo_setup_tc,
>  };
>
> +static void am65_cpsw_disable_phy(struct phy *phy)
> +{
> +       phy_power_off(phy);
> +       phy_exit(phy);
> +}
> +
> +static int am65_cpsw_enable_phy(struct phy *phy)
> +{
> +       int ret;
> +
> +       ret = phy_init(phy);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = phy_power_on(phy);
> +       if (ret < 0) {
> +               phy_exit(phy);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static void am65_cpsw_disable_serdes_phy(struct am65_cpsw_common *common)
> +{
> +       struct am65_cpsw_port *port;
> +       struct phy *phy;
> +       int i;
> +
> +       for (i = 0; i < common->port_num; i++) {
> +               port = &common->ports[i];
> +               phy = port->slave.serdes_phy;
> +               if (phy)
> +                       am65_cpsw_disable_phy(phy);
> +       }
> +}
> +
> +static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *port_np,
> +                                    struct am65_cpsw_port *port)
> +{
> +       const char *name = "serdes-phy";
> +       struct phy *phy;
> +       int ret;
> +
> +       phy = devm_of_phy_get(dev, port_np, name);
> +       if (PTR_ERR(phy) == -ENODEV)
> +               return 0;
> +
> +       /* Serdes PHY exists. Store it. */

"phy" may be a different error here (e.g. -EPROBE_DEFER)...

> +       port->slave.serdes_phy = phy;
> +
> +       ret =  am65_cpsw_enable_phy(phy);

... so it will crash when dereferencing phy in phy_init().

I think you want to add an extra check above:

    if (IS_ERR(phy))
            return PTR_ERR(phy);

> +       if (ret < 0)
> +               goto err_phy;
> +
> +       return 0;
> +
> +err_phy:
> +       devm_phy_put(dev, phy);
> +       return ret;
> +}
> +
>  static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned int mode,
>                                       const struct phylink_link_state *state)
>  {
> @@ -1959,6 +2021,11 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)

Right out of context we have:

                port->slave.ifphy = devm_of_phy_get(dev, port_np, NULL);
                if (IS_ERR(port->slave.ifphy)) {
                        ret = PTR_ERR(port->slave.ifphy);
                        dev_err(dev, "%pOF error retrieving port phy: %d\n",
                                port_np, ret);

So if there is only one PHY (named "serdes-phy") in DT, it will be
used for both ifphy and serdes_phy. Is that intentional?

>                         goto of_node_put;
>                 }
>
> +               /* Initialize the Serdes PHY for the port */
> +               ret = am65_cpsw_init_serdes_phy(dev, port_np, port);
> +               if (ret)
> +                       return ret;
> +
>                 port->slave.mac_only =
>                                 of_property_read_bool(port_np, "ti,mac-only");
>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH net-next v6 3/3] net: ethernet: ti: am65-cpsw: Add support for SERDES configuration

[Siddharth Vadapalli]

Hello Geert,

Thank you for reviewing the patch.

On 17/01/23 19:25, Geert Uytterhoeven wrote:
> Hi Siddharth,
> 
> On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
>> Use PHY framework APIs to initialize the SERDES PHY connected to CPSW MAC.
>>
>> Define the functions am65_cpsw_disable_phy(), am65_cpsw_enable_phy(),
>> am65_cpsw_disable_serdes_phy() and am65_cpsw_enable_serdes_phy().
>>
>> Add new member "serdes_phy" to struct "am65_cpsw_slave_data" to store the
>> SERDES PHY for each port, if it exists. Use it later while disabling the
>> SERDES PHY for each port.
>>
>> Power on and initialize the SerDes PHY in am65_cpsw_nuss_init_slave_ports()
>> by invoking am65_cpsw_enable_serdes_phy().
>>
>> Power off the SerDes PHY in am65_cpsw_nuss_remove() by invoking
>> am65_cpsw_disable_serdes_phy().
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> 
> Thanks for your patch, which is now commit dab2b265dd23ef8f ("net:
> ethernet: ti: am65-cpsw: Add support for SERDES configuration")
> in net-next.
> 
>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> @@ -1416,6 +1416,68 @@ static const struct net_device_ops am65_cpsw_nuss_netdev_ops = {
>>         .ndo_setup_tc           = am65_cpsw_qos_ndo_setup_tc,
>>  };
>>
>> +static void am65_cpsw_disable_phy(struct phy *phy)
>> +{
>> +       phy_power_off(phy);
>> +       phy_exit(phy);
>> +}
>> +
>> +static int am65_cpsw_enable_phy(struct phy *phy)
>> +{
>> +       int ret;
>> +
>> +       ret = phy_init(phy);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ret = phy_power_on(phy);
>> +       if (ret < 0) {
>> +               phy_exit(phy);
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void am65_cpsw_disable_serdes_phy(struct am65_cpsw_common *common)
>> +{
>> +       struct am65_cpsw_port *port;
>> +       struct phy *phy;
>> +       int i;
>> +
>> +       for (i = 0; i < common->port_num; i++) {
>> +               port = &common->ports[i];
>> +               phy = port->slave.serdes_phy;
>> +               if (phy)
>> +                       am65_cpsw_disable_phy(phy);
>> +       }
>> +}
>> +
>> +static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *port_np,
>> +                                    struct am65_cpsw_port *port)
>> +{
>> +       const char *name = "serdes-phy";
>> +       struct phy *phy;
>> +       int ret;
>> +
>> +       phy = devm_of_phy_get(dev, port_np, name);
>> +       if (PTR_ERR(phy) == -ENODEV)
>> +               return 0;
>> +
>> +       /* Serdes PHY exists. Store it. */
> 
> "phy" may be a different error here (e.g. -EPROBE_DEFER)...

The Serdes is automatically configured for multi-link protocol (Example: PCIe +
QSGMII) by the Serdes driver, due to which it is not necessary to invoke the
Serdes configuration via phy_init(). However, for single-link protocol (Example:
Serdes has to be configured only for SGMII), the Serdes driver doesn't configure
the Serdes unless requested. For this case, the am65-cpsw driver explicitly
invokes phy_init() for the Serdes to be configured, by looking up the optional
device-tree phy named "serdes-phy". For this reason, the above section of code
is actually emulating a non-existent "devm_of_phy_optional_get()". The
"devm_of_phy_optional_get()" function is similar to the
"devm_phy_optional_get()" function in the sense that the "serdes-phy" phy in the
device-tree is optional and it is not truly an error if the property isn't present.

Thank you for pointing out that if the Serdes driver is built as a module and
the am65-cpsw driver runs first, then the "phy" returned for "serdes-phy" will
be "-EPROBE_DEFER".

> 
>> +       port->slave.serdes_phy = phy;
>> +
>> +       ret =  am65_cpsw_enable_phy(phy);
> 
> ... so it will crash when dereferencing phy in phy_init().
> 
> I think you want to add an extra check above:
> 
>     if (IS_ERR(phy))
>             return PTR_ERR(phy);

Please let me know if posting a "Fixes" patch for fixing this net-next commit is
the right process to address this.

> 
>> +       if (ret < 0)
>> +               goto err_phy;
>> +
>> +       return 0;
>> +
>> +err_phy:
>> +       devm_phy_put(dev, phy);
>> +       return ret;
>> +}
>> +
>>  static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned int mode,
>>                                       const struct phylink_link_state *state)
>>  {
>> @@ -1959,6 +2021,11 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
> 
> Right out of context we have:
> 
>                 port->slave.ifphy = devm_of_phy_get(dev, port_np, NULL);
>                 if (IS_ERR(port->slave.ifphy)) {
>                         ret = PTR_ERR(port->slave.ifphy);
>                         dev_err(dev, "%pOF error retrieving port phy: %d\n",
>                                 port_np, ret);
> 
> So if there is only one PHY (named "serdes-phy") in DT, it will be
> used for both ifphy and serdes_phy. Is that intentional?

The PHY corresponding to "ifphy" is meant to be the CPSW MAC's PHY and not the
Serdes PHY. The CPSW MAC's PHY is configured by the
drivers/phy/ti/phy-gmii-sel.c driver and this is NOT an optional PHY, unlike the
Serdes PHY. Therefore, it is assumed that the CPSW MAC's PHY is always provided
in the device-tree, while the Serdes PHY is optional, depending on whether the
Serdes is being configured for single-link protocol or multi-link protocol.
Please let me know if this appears to be an issue and I will fix it based on
your suggestion.

Regards,
Siddharth.

Re: [PATCH net-next v6 3/3] net: ethernet: ti: am65-cpsw: Add support for SERDES configuration

[Geert Uytterhoeven]

Hi Siddarth,

On Wed, Jan 18, 2023 at 6:48 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
> On 17/01/23 19:25, Geert Uytterhoeven wrote:
> > On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
> >> Use PHY framework APIs to initialize the SERDES PHY connected to CPSW MAC.
> >>
> >> Define the functions am65_cpsw_disable_phy(), am65_cpsw_enable_phy(),
> >> am65_cpsw_disable_serdes_phy() and am65_cpsw_enable_serdes_phy().
> >>
> >> Add new member "serdes_phy" to struct "am65_cpsw_slave_data" to store the
> >> SERDES PHY for each port, if it exists. Use it later while disabling the
> >> SERDES PHY for each port.
> >>
> >> Power on and initialize the SerDes PHY in am65_cpsw_nuss_init_slave_ports()
> >> by invoking am65_cpsw_enable_serdes_phy().
> >>
> >> Power off the SerDes PHY in am65_cpsw_nuss_remove() by invoking
> >> am65_cpsw_disable_serdes_phy().
> >>
> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> >
> > Thanks for your patch, which is now commit dab2b265dd23ef8f ("net:
> > ethernet: ti: am65-cpsw: Add support for SERDES configuration")
> > in net-next.
> >
> >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c

> >> +static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *port_np,
> >> +                                    struct am65_cpsw_port *port)
> >> +{
> >> +       const char *name = "serdes-phy";
> >> +       struct phy *phy;
> >> +       int ret;
> >> +
> >> +       phy = devm_of_phy_get(dev, port_np, name);
> >> +       if (PTR_ERR(phy) == -ENODEV)
> >> +               return 0;
> >> +
> >> +       /* Serdes PHY exists. Store it. */
> >
> > "phy" may be a different error here (e.g. -EPROBE_DEFER)...
>
> The Serdes is automatically configured for multi-link protocol (Example: PCIe +
> QSGMII) by the Serdes driver, due to which it is not necessary to invoke the
> Serdes configuration via phy_init(). However, for single-link protocol (Example:
> Serdes has to be configured only for SGMII), the Serdes driver doesn't configure
> the Serdes unless requested. For this case, the am65-cpsw driver explicitly
> invokes phy_init() for the Serdes to be configured, by looking up the optional
> device-tree phy named "serdes-phy". For this reason, the above section of code
> is actually emulating a non-existent "devm_of_phy_optional_get()". The
> "devm_of_phy_optional_get()" function is similar to the
> "devm_phy_optional_get()" function in the sense that the "serdes-phy" phy in the
> device-tree is optional and it is not truly an error if the property isn't present.

Yeah, I noticed while adding devm_phy_optional_get(), and looking for
possible users.
See "[PATCH treewide 0/7] phy: Add devm_of_phy_optional_get() helper"
https://lore.kernel.org/all/cover.1674036164.git.geert+renesas@glider.be

> Thank you for pointing out that if the Serdes driver is built as a module and
> the am65-cpsw driver runs first, then the "phy" returned for "serdes-phy" will
> be "-EPROBE_DEFER".
>
> >
> >> +       port->slave.serdes_phy = phy;
> >> +
> >> +       ret =  am65_cpsw_enable_phy(phy);
> >
> > ... so it will crash when dereferencing phy in phy_init().
> >
> > I think you want to add an extra check above:
> >
> >     if (IS_ERR(phy))
> >             return PTR_ERR(phy);
>
> Please let me know if posting a "Fixes" patch for fixing this net-next commit is
> the right process to address this.

I think it is, as devm_of_phy_optional_get() might not make it in time.

> >> @@ -1959,6 +2021,11 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
> >
> > Right out of context we have:
> >
> >                 port->slave.ifphy = devm_of_phy_get(dev, port_np, NULL);
> >                 if (IS_ERR(port->slave.ifphy)) {
> >                         ret = PTR_ERR(port->slave.ifphy);
> >                         dev_err(dev, "%pOF error retrieving port phy: %d\n",
> >                                 port_np, ret);
> >
> > So if there is only one PHY (named "serdes-phy") in DT, it will be
> > used for both ifphy and serdes_phy. Is that intentional?
>
> The PHY corresponding to "ifphy" is meant to be the CPSW MAC's PHY and not the
> Serdes PHY. The CPSW MAC's PHY is configured by the
> drivers/phy/ti/phy-gmii-sel.c driver and this is NOT an optional PHY, unlike the
> Serdes PHY. Therefore, it is assumed that the CPSW MAC's PHY is always provided
> in the device-tree, while the Serdes PHY is optional, depending on whether the
> Serdes is being configured for single-link protocol or multi-link protocol.
> Please let me know if this appears to be an issue and I will fix it based on
> your suggestion.

Hence this should be documented in the DT bindings. Please document
there can be 1 or 2 phys, with an optional "phys-names" property,
listing "ifphy" and "serdes-phy" (the DT people might request a rename).

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH net-next v6 3/3] net: ethernet: ti: am65-cpsw: Add support for SERDES configuration

[Siddharth Vadapalli]

Hello Geert,

On 18/01/23 15:57, Geert Uytterhoeven wrote:
> Hi Siddarth,
> 
> On Wed, Jan 18, 2023 at 6:48 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
>> On 17/01/23 19:25, Geert Uytterhoeven wrote:
>>> On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
>>>> Use PHY framework APIs to initialize the SERDES PHY connected to CPSW MAC.
>>>>
>>>> Define the functions am65_cpsw_disable_phy(), am65_cpsw_enable_phy(),
>>>> am65_cpsw_disable_serdes_phy() and am65_cpsw_enable_serdes_phy().
>>>>
>>>> Add new member "serdes_phy" to struct "am65_cpsw_slave_data" to store the
>>>> SERDES PHY for each port, if it exists. Use it later while disabling the
>>>> SERDES PHY for each port.
>>>>
>>>> Power on and initialize the SerDes PHY in am65_cpsw_nuss_init_slave_ports()
>>>> by invoking am65_cpsw_enable_serdes_phy().
>>>>
>>>> Power off the SerDes PHY in am65_cpsw_nuss_remove() by invoking
>>>> am65_cpsw_disable_serdes_phy().
>>>>
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>
>>> Thanks for your patch, which is now commit dab2b265dd23ef8f ("net:
>>> ethernet: ti: am65-cpsw: Add support for SERDES configuration")
>>> in net-next.
>>>
>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> 
>>>> +static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *port_np,
>>>> +                                    struct am65_cpsw_port *port)
>>>> +{
>>>> +       const char *name = "serdes-phy";
>>>> +       struct phy *phy;
>>>> +       int ret;
>>>> +
>>>> +       phy = devm_of_phy_get(dev, port_np, name);
>>>> +       if (PTR_ERR(phy) == -ENODEV)
>>>> +               return 0;
>>>> +
>>>> +       /* Serdes PHY exists. Store it. */
>>>
>>> "phy" may be a different error here (e.g. -EPROBE_DEFER)...
>>
>> The Serdes is automatically configured for multi-link protocol (Example: PCIe +
>> QSGMII) by the Serdes driver, due to which it is not necessary to invoke the
>> Serdes configuration via phy_init(). However, for single-link protocol (Example:
>> Serdes has to be configured only for SGMII), the Serdes driver doesn't configure
>> the Serdes unless requested. For this case, the am65-cpsw driver explicitly
>> invokes phy_init() for the Serdes to be configured, by looking up the optional
>> device-tree phy named "serdes-phy". For this reason, the above section of code
>> is actually emulating a non-existent "devm_of_phy_optional_get()". The
>> "devm_of_phy_optional_get()" function is similar to the
>> "devm_phy_optional_get()" function in the sense that the "serdes-phy" phy in the
>> device-tree is optional and it is not truly an error if the property isn't present.
> 
> Yeah, I noticed while adding devm_phy_optional_get(), and looking for
> possible users.
> See "[PATCH treewide 0/7] phy: Add devm_of_phy_optional_get() helper"
> https://lore.kernel.org/all/cover.1674036164.git.geert+renesas@glider.be

Thank you for working on this.

> 
>> Thank you for pointing out that if the Serdes driver is built as a module and
>> the am65-cpsw driver runs first, then the "phy" returned for "serdes-phy" will
>> be "-EPROBE_DEFER".
>>
>>>
>>>> +       port->slave.serdes_phy = phy;
>>>> +
>>>> +       ret =  am65_cpsw_enable_phy(phy);
>>>
>>> ... so it will crash when dereferencing phy in phy_init().
>>>
>>> I think you want to add an extra check above:
>>>
>>>     if (IS_ERR(phy))
>>>             return PTR_ERR(phy);
>>
>> Please let me know if posting a "Fixes" patch for fixing this net-next commit is
>> the right process to address this.
> 
> I think it is, as devm_of_phy_optional_get() might not make it in time.

I posted the patch at:
https://lore.kernel.org/r/20230118112136.213061-1-s-vadapalli@ti.com

> 
>>>> @@ -1959,6 +2021,11 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
>>>
>>> Right out of context we have:
>>>
>>>                 port->slave.ifphy = devm_of_phy_get(dev, port_np, NULL);
>>>                 if (IS_ERR(port->slave.ifphy)) {
>>>                         ret = PTR_ERR(port->slave.ifphy);
>>>                         dev_err(dev, "%pOF error retrieving port phy: %d\n",
>>>                                 port_np, ret);
>>>
>>> So if there is only one PHY (named "serdes-phy") in DT, it will be
>>> used for both ifphy and serdes_phy. Is that intentional?
>>
>> The PHY corresponding to "ifphy" is meant to be the CPSW MAC's PHY and not the
>> Serdes PHY. The CPSW MAC's PHY is configured by the
>> drivers/phy/ti/phy-gmii-sel.c driver and this is NOT an optional PHY, unlike the
>> Serdes PHY. Therefore, it is assumed that the CPSW MAC's PHY is always provided
>> in the device-tree, while the Serdes PHY is optional, depending on whether the
>> Serdes is being configured for single-link protocol or multi-link protocol.
>> Please let me know if this appears to be an issue and I will fix it based on
>> your suggestion.
> 
> Hence this should be documented in the DT bindings. Please document
> there can be 1 or 2 phys, with an optional "phys-names" property,
> listing "ifphy" and "serdes-phy" (the DT people might request a rename).

I will work on this.

Regards,
Siddharth.

Re: [PATCH net-next v6 0/3] Add support for QSGMII mode for J721e CPSW9G to am65-cpsw driver

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Wed, 4 Jan 2023 16:04:29 +0530 you wrote:
> Add compatible to am65-cpsw driver for J721e CPSW9G, which contains 8
> external ports and 1 internal host port.
> 
> Add support to power on and power off the SERDES PHY which is used by the
> CPSW MAC.
> 
> =========
> Changelog
> =========
> v5 -> v6:
> 1. Add member "serdes_phy" in struct "am65_cpsw_slave_data" to store the
>    SERDES PHY for each port, if present. This is done to cache the SERDES
>    PHY beforehand, without depending on devm_of_phy_get().
> 2. Rebase series on net-next tree.
> 
> [...]

Here is the summary with links:
  - [net-next,v6,1/3] dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G support
    https://git.kernel.org/netdev/net-next/c/c85b53e32c8e
  - [net-next,v6,2/3] net: ethernet: ti: am65-cpsw: Enable QSGMII mode for J721e CPSW9G
    https://git.kernel.org/netdev/net-next/c/944131fa65d7
  - [net-next,v6,3/3] net: ethernet: ti: am65-cpsw: Add support for SERDES configuration
    https://git.kernel.org/netdev/net-next/c/dab2b265dd23

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html