Date:   Fri, 2 Aug 2019 17:22:26 +0200
From:   Maxime Chevallier <maxime.chevallier@...tlin.com>
To:     Matt Pelland <mpelland@...rry.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        antoine.tenart@...tlin.com
Subject: Re: [PATCH 2/2] net: mvpp2: support multiple comphy lanes

Hello Matt,

On Thu,  1 Aug 2019 16:45:23 -0400
Matt Pelland <mpelland@...rry.com> wrote:

>mvpp 2.2 supports RXAUI which requires a pair of serdes lanes instead of
>the usual single lane required by other interface modes. This patch
>expands the number of lanes that can be associated to a port so that
>both lanes are correctly configured at the appropriate times when RXAUI
>is in use.

Thanks for the patch, it's nice to see RXAUI support moving forward.

I'll give your patches a test on my setup, although I never really got
something stable, it might simply be due to the particular hardware I
have access to.

I do have some comments below :

>Signed-off-by: Matt Pelland <mpelland@...rry.com>
>---
> drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  7 +-
> .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 66 +++++++++++++------
> 2 files changed, 53 insertions(+), 20 deletions(-)
>
>diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
>index 256e7c796631..9ae2b3d9d0c7 100644
>--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
>+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
>@@ -655,6 +655,11 @@
> #define MVPP2_F_LOOPBACK		BIT(0)
> #define MVPP2_F_DT_COMPAT		BIT(1)
> 
>+/* MVPP22 supports RXAUI which requires two comphy lanes, all other modes
>+ * require one.
>+ */
>+#define MVPP22_MAX_COMPHYS		2

This driver is "supposed" to support XAUI too, at least it appears in
the list of modes we handle. XAUI uses 4 lanes, maybe we could bump the
max number of lanes to 4.

> /* Marvell tag types */
> enum mvpp2_tag_type {
> 	MVPP2_TAG_TYPE_NONE = 0,
>@@ -935,7 +940,7 @@ struct mvpp2_port {
> 	phy_interface_t phy_interface;
> 	struct phylink *phylink;
> 	struct phylink_config phylink_config;
>-	struct phy *comphy;
>+	struct phy *comphys[MVPP22_MAX_COMPHYS];
> 
> 	struct mvpp2_bm_pool *pool_long;
> 	struct mvpp2_bm_pool *pool_short;
>diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>index 8b633af4a684..97dfe2e71b03 100644
>--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>@@ -1186,17 +1186,40 @@ static void mvpp22_gop_setup_irq(struct mvpp2_port *port)
>  */
> static int mvpp22_comphy_init(struct mvpp2_port *port)
> {
>-	int ret;
>+	int i, ret;
> 
>-	if (!port->comphy)
>-		return 0;
>+	for (i = 0; i < ARRAY_SIZE(port->comphys); i++) {
>+		if (!port->comphys[i])
>+			return 0;
> 
>-	ret = phy_set_mode_ext(port->comphy, PHY_MODE_ETHERNET,
>-			       port->phy_interface);
>-	if (ret)
>-		return ret;
>+		ret = phy_set_mode_ext(port->comphys[i],
>+				       PHY_MODE_ETHERNET,
>+				       port->phy_interface);
>+		if (ret)
>+			return ret;
> 
>-	return phy_power_on(port->comphy);
>+		ret = phy_power_on(port->comphys[i]);
>+		if (ret)
>+			return ret;
>+	}
>+
>+	return 0;
>+}

It could be good to add some sanity check, to make sure we do have 2
comphy lanes available when configuring RXAUI mode (and 4 for XAUI).

>+static int mvpp22_comphy_deinit(struct mvpp2_port *port)
>+{
>+	int i, ret;
>+
>+	for (i = 0; i < ARRAY_SIZE(port->comphys); i++) {
>+		if (!port->comphys[i])
>+			return 0;
>+
>+		ret = phy_power_off(port->comphys[i]);
>+		if (ret)
>+			return ret;
>+	}
>+
>+	return 0;
> }
> 
> static void mvpp2_port_enable(struct mvpp2_port *port)
>@@ -3375,7 +3398,9 @@ static void mvpp2_stop_dev(struct mvpp2_port *port)
> 
> 	if (port->phylink)
> 		phylink_stop(port->phylink);
>-	phy_power_off(port->comphy);
>+
>+	if (port->priv->hw_version == MVPP22)
>+		mvpp22_comphy_deinit(port);
> }
> 
> static int mvpp2_check_ringparam_valid(struct net_device *dev,
>@@ -4947,7 +4972,7 @@ static void mvpp2_mac_config(struct phylink_config *config, unsigned int mode,
> 		port->phy_interface = state->interface;
> 
> 		/* Reconfigure the serdes lanes */
>-		phy_power_off(port->comphy);
>+		mvpp22_comphy_deinit(port);
> 		mvpp22_mode_reconfigure(port);
> 	}
> 
>@@ -5038,7 +5063,6 @@ static int mvpp2_port_probe(struct platform_device *pdev,
> 			    struct fwnode_handle *port_fwnode,
> 			    struct mvpp2 *priv)
> {
>-	struct phy *comphy = NULL;
> 	struct mvpp2_port *port;
> 	struct mvpp2_port_pcpu *port_pcpu;
> 	struct device_node *port_node = to_of_node(port_fwnode);
>@@ -5085,14 +5109,20 @@ static int mvpp2_port_probe(struct platform_device *pdev,
> 		goto err_free_netdev;
> 	}
> 
>+	port = netdev_priv(dev);
>+
> 	if (port_node) {
>-		comphy = devm_of_phy_get(&pdev->dev, port_node, NULL);
>-		if (IS_ERR(comphy)) {
>-			if (PTR_ERR(comphy) == -EPROBE_DEFER) {
>-				err = -EPROBE_DEFER;
>-				goto err_free_netdev;
>+		for (i = 0; i < ARRAY_SIZE(port->comphys); i++) {
>+			port->comphys[i] = devm_of_phy_get_by_index(&pdev->dev,
>+								    port_node,
>+								    i);
>+			if (IS_ERR(port->comphys[i])) {
>+				err = PTR_ERR(port->comphys[i]);
>+				port->comphys[i] = NULL;
>+				if (err == -EPROBE_DEFER)
>+					goto err_free_netdev;
>+				err = 0;
> 			}
>-			comphy = NULL;
> 		}
> 	}
> 
>@@ -5107,7 +5137,6 @@ static int mvpp2_port_probe(struct platform_device *pdev,
> 	dev->netdev_ops = &mvpp2_netdev_ops;
> 	dev->ethtool_ops = &mvpp2_eth_tool_ops;
> 
>-	port = netdev_priv(dev);
> 	port->dev = dev;
> 	port->fwnode = port_fwnode;
> 	port->has_phy = !!of_find_property(port_node, "phy", NULL);
>@@ -5144,7 +5173,6 @@ static int mvpp2_port_probe(struct platform_device *pdev,
> 
> 	port->of_node = port_node;
> 	port->phy_interface = phy_mode;
>-	port->comphy = comphy;
> 
> 	if (priv->hw_version == MVPP21) {
> 		port->base = devm_platform_ioremap_resource(pdev, 2 + id);



-- 
Maxime Chevallier, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Powered by blists - more mailing lists