[2/8] net: ethernet: ti: am65-cpsw: Add support for SERDES configuration

Commit Message

Siddharth Vadapalli Sept. 14, 2022, 9:50 a.m. UTC

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

Define the functions am65_cpsw_init_phy(), am65_cpsw_enable_phy() and
am65_cpsw_disable_phy() and invoke in am65_cpsw_nuss_init_slave_ports(),
am65_cpsw_mac_link_up() and am65_cpsw_mac_link_down() respectively.

Signed-off-by: Siddharth Vadapalli <[e-mail]>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 55 ++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Comments

Russell King (Oracle) Sept. 14, 2022, 3:37 p.m. UTC | #1

On Wed, Sep 14, 2022 at 03:20:47PM +0530, Siddharth Vadapalli wrote:
> @@ -1427,6 +1471,9 @@ static void am65_cpsw_nuss_mac_link_down(struct phylink_config *config, unsigned
>  	struct net_device *ndev = port->ndev;
>  	int tmo;
>  
> +	/* disable phy */
> +	am65_cpsw_disable_phy(port->slave.ifphy);
> +

This seems really strange. If you have a serdes interface which
presumably supports SGMII, 1000base-X etc, then link status is sent
across the serdes interface. If you power down the serdes, then you
can't receive the link status, and so mac_link_up() won't be called.

Are you really sure you want to be enabling and disabling the PHY
in mac_link_down()/mac_link_up() ?

Siddharth Vadapalli Sept. 15, 2022, 8:36 a.m. UTC | #2

Hello Russell,

On 14/09/22 21:07, Russell King (Oracle) wrote:
> On Wed, Sep 14, 2022 at 03:20:47PM +0530, Siddharth Vadapalli wrote:
>> @@ -1427,6 +1471,9 @@ static void am65_cpsw_nuss_mac_link_down(struct phylink_config *config, unsigned
>>  	struct net_device *ndev = port->ndev;
>>  	int tmo;
>>  
>> +	/* disable phy */
>> +	am65_cpsw_disable_phy(port->slave.ifphy);
>> +
> 
> This seems really strange. If you have a serdes interface which
> presumably supports SGMII, 1000base-X etc, then link status is sent
> across the serdes interface. If you power down the serdes, then you
> can't receive the link status, and so mac_link_up() won't be called.
> 
> Are you really sure you want to be enabling and disabling the PHY
> in mac_link_down()/mac_link_up() ?

Thank you for reviewing the patch. The PHY passed to the
"am65_cpsw_disable_phy()" and "am65_cpsw_disable_phy()" functions within
the "am65_cpsw_nuss_mac_link_down()" and "am65_cpsw_nuss_mac_link_up()"
functions respectively, is the CPSW ethernet MAC's PHY and not the
SERDES PHY. The SERDES PHY is powered on through the function call to
the "am65_cpsw_init_phy()" function.

The calls to the functions "am65_cpsw_enable_phy()" and
"am65_cpsw_disable_phy()" within the "am65_cpsw_nuss_mac_link_up()" and
"am65_cpsw_nuss_mac_link_down()" functions respectively, try to power on
and power off the CPSW ethernet MAC's phy. Looking at it again,they do
nothing, since the driver corresponding to the ethernet MAC's PHY which
happens to be drivers/phy/ti/phy-gmii-sel.c, does not provide any
methods to power on and power off the ethernet MAC's PHY. I have just
realized that this is stale code and will remove it in the v2 series.

Also, I realize now that I did not invoke "am65_cpsw_disable_phy()" on
the SERDES PHY in the driver's remove function. I will fix this in the
v2 series.

Regards,
Siddharth.

Patch

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 7ef5d8208a4e..4e06def3b0de 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -1404,6 +1404,50 @@  static const struct net_device_ops am65_cpsw_nuss_netdev_ops = {
 	.ndo_get_devlink_port   = am65_cpsw_ndo_get_devlink_port,
 };
 
+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 int am65_cpsw_init_phy(struct device *dev, struct device_node *port_np)
+{
+	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;
+
+	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)
 {
@@ -1427,6 +1471,9 @@  static void am65_cpsw_nuss_mac_link_down(struct phylink_config *config, unsigned
 	struct net_device *ndev = port->ndev;
 	int tmo;
 
+	/* disable phy */
+	am65_cpsw_disable_phy(port->slave.ifphy);
+
 	/* disable forwarding */
 	cpsw_ale_control_set(common->ale, port->port_id, ALE_PORT_STATE, ALE_PORT_STATE_DISABLE);
 
@@ -1472,6 +1519,9 @@  static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
 
 	cpsw_sl_ctl_set(port->slave.mac_sl, mac_control);
 
+	/* enable phy */
+	am65_cpsw_enable_phy(port->slave.ifphy);
+
 	/* enable forwarding */
 	cpsw_ale_control_set(common->ale, port->port_id, ALE_PORT_STATE, ALE_PORT_STATE_FORWARD);
 
@@ -1881,6 +1931,11 @@  static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
 			goto of_node_put;
 		}
 
+		/* Initialize the phy for the port */
+		ret = am65_cpsw_init_phy(dev, port_np);
+		if (ret)
+			return ret;
+
 		port->slave.mac_only =
 				of_property_read_bool(port_np, "ti,mac-only");