[PATCH v2 net-next 0/2] net: mvpp2: Implement RXAUI Support

[PATCH v2 net-next 0/2] net: mvpp2: Implement RXAUI Support

[Matt Pelland]

This patch set implements support for configuring Marvell's mvpp2 hardware for
RXAUI operation. There are two other patches necessary for this to work
correctly that concern Marvell's cp110 comphy that were emailed to the general
linux-kernel mailing list earlier on. I can post them here if need be. This
patch set was successfully tested on both a Marvell Armada 7040 based platform
as well as an Armada 8040 based platform.

Changes since v1:

- Use reverse christmas tree formatting for all modified declaration blocks.
- Bump MVP22_MAX_COMPHYS to 4 to allow for XAUI operation.
- Implement comphy sanity checking.

Matt Pelland (2):
  net: mvpp2: implement RXAUI support
  net: mvpp2: support multiple comphy lanes

 drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |   8 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 129 ++++++++++++++----
 2 files changed, 110 insertions(+), 27 deletions(-)

-- 
2.21.0

[PATCH v2 net-next 1/2] net: mvpp2: implement RXAUI support

[Matt Pelland]

Marvell's mvpp2 packet processor supports RXAUI on port zero in a
similar manner to the existing 10G protocols that have already been
implemented. This patch implements the miscellaneous extra configuration
steps required for RXAUI operation.

Signed-off-by: Matt Pelland <mpelland@starry.com>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  1 +
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 32 +++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index 4d9564ba68f6..256e7c796631 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -481,6 +481,7 @@
 #define MVPP22_XLG_CTRL4_REG			0x184
 #define     MVPP22_XLG_CTRL4_FWD_FC		BIT(5)
 #define     MVPP22_XLG_CTRL4_FWD_PFC		BIT(6)
+#define     MVPP22_XLG_CTRL4_USE_XPCS		BIT(8)
 #define     MVPP22_XLG_CTRL4_MACMODSELECT_GMAC	BIT(12)
 #define     MVPP22_XLG_CTRL4_EN_IDLE_CHECK	BIT(14)
 
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 74fd9e171865..1a5037a398fc 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -980,6 +980,7 @@ mvpp2_shared_interrupt_mask_unmask(struct mvpp2_port *port, bool mask)
 static bool mvpp2_is_xlg(phy_interface_t interface)
 {
 	return interface == PHY_INTERFACE_MODE_10GKR ||
+	       interface == PHY_INTERFACE_MODE_RXAUI ||
 	       interface == PHY_INTERFACE_MODE_XAUI;
 }
 
@@ -1020,6 +1021,29 @@ static void mvpp22_gop_init_sgmii(struct mvpp2_port *port)
 	}
 }
 
+static void mvpp22_gop_init_rxaui(struct mvpp2_port *port)
+{
+	struct mvpp2 *priv = port->priv;
+	void __iomem *xpcs;
+	u32 val;
+
+	xpcs = priv->iface_base + MVPP22_XPCS_BASE(port->gop_id);
+
+	val = readl(xpcs + MVPP22_XPCS_CFG0);
+	val &= ~MVPP22_XPCS_CFG0_RESET_DIS;
+	writel(val, xpcs + MVPP22_XPCS_CFG0);
+
+	val = readl(xpcs + MVPP22_XPCS_CFG0);
+	val &= ~(MVPP22_XPCS_CFG0_PCS_MODE(0x3) |
+		 MVPP22_XPCS_CFG0_ACTIVE_LANE(0x3));
+	val |= MVPP22_XPCS_CFG0_ACTIVE_LANE(2);
+	writel(val, xpcs + MVPP22_XPCS_CFG0);
+
+	val = readl(xpcs + MVPP22_XPCS_CFG0);
+	val |= MVPP22_XPCS_CFG0_RESET_DIS;
+	writel(val, xpcs + MVPP22_XPCS_CFG0);
+}
+
 static void mvpp22_gop_init_10gkr(struct mvpp2_port *port)
 {
 	struct mvpp2 *priv = port->priv;
@@ -1065,6 +1089,9 @@ static int mvpp22_gop_init(struct mvpp2_port *port)
 	case PHY_INTERFACE_MODE_2500BASEX:
 		mvpp22_gop_init_sgmii(port);
 		break;
+	case PHY_INTERFACE_MODE_RXAUI:
+		mvpp22_gop_init_rxaui(port);
+		break;
 	case PHY_INTERFACE_MODE_10GKR:
 		if (port->gop_id != 0)
 			goto invalid_conf;
@@ -4567,6 +4594,7 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
 	switch (state->interface) {
 	case PHY_INTERFACE_MODE_10GKR:
 	case PHY_INTERFACE_MODE_XAUI:
+	case PHY_INTERFACE_MODE_RXAUI:
 		if (port->gop_id != 0)
 			goto empty_set;
 		break;
@@ -4589,6 +4617,7 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
 	switch (state->interface) {
 	case PHY_INTERFACE_MODE_10GKR:
 	case PHY_INTERFACE_MODE_XAUI:
+	case PHY_INTERFACE_MODE_RXAUI:
 	case PHY_INTERFACE_MODE_NA:
 		if (port->gop_id == 0) {
 			phylink_set(mask, 10000baseT_Full);
@@ -4741,6 +4770,9 @@ static void mvpp2_xlg_config(struct mvpp2_port *port, unsigned int mode,
 		   MVPP22_XLG_CTRL4_EN_IDLE_CHECK);
 	ctrl4 |= MVPP22_XLG_CTRL4_FWD_FC | MVPP22_XLG_CTRL4_FWD_PFC;
 
+	if (state->interface == PHY_INTERFACE_MODE_RXAUI)
+		ctrl4 |= MVPP22_XLG_CTRL4_USE_XPCS;
+
 	if (old_ctrl0 != ctrl0)
 		writel(ctrl0, port->base + MVPP22_XLG_CTRL0_REG);
 	if (old_ctrl4 != ctrl4)
-- 
2.21.0

Re: [PATCH v2 net-next 1/2] net: mvpp2: implement RXAUI support

[Antoine Tenart]

Hello Matt,

On Thu, Aug 08, 2019 at 07:06:05PM -0400, Matt Pelland wrote:
>  
> +static void mvpp22_gop_init_rxaui(struct mvpp2_port *port)
> +{
> +	struct mvpp2 *priv = port->priv;
> +	void __iomem *xpcs;
> +	u32 val;
> +
> +	xpcs = priv->iface_base + MVPP22_XPCS_BASE(port->gop_id);
> +
> +	val = readl(xpcs + MVPP22_XPCS_CFG0);
> +	val &= ~MVPP22_XPCS_CFG0_RESET_DIS;
> +	writel(val, xpcs + MVPP22_XPCS_CFG0);

The reset logic of the various blocks in PPv2 is handled outside of the
GoP init functions. You should only modify the XPCS configuration here,
without taking care of the reset. See mvpp22_pcs_reset_assert() and
mvpp22_pcs_reset_deassert().

Note that gop_init() is always called with the XPCS reset asserted.

>  static void mvpp22_gop_init_10gkr(struct mvpp2_port *port)
>  {
>  	struct mvpp2 *priv = port->priv;
> @@ -1065,6 +1089,9 @@ static int mvpp22_gop_init(struct mvpp2_port *port)
>  	case PHY_INTERFACE_MODE_2500BASEX:
>  		mvpp22_gop_init_sgmii(port);
>  		break;
> +	case PHY_INTERFACE_MODE_RXAUI:
> +		mvpp22_gop_init_rxaui(port);
> +		break;

Isn't RXAUI only supported on port #0? (Such as the 10GKR mode below).

>  	case PHY_INTERFACE_MODE_10GKR:
>  		if (port->gop_id != 0)
>  			goto invalid_conf;


>  		   MVPP22_XLG_CTRL4_EN_IDLE_CHECK);
>  	ctrl4 |= MVPP22_XLG_CTRL4_FWD_FC | MVPP22_XLG_CTRL4_FWD_PFC;
>  
> +	if (state->interface == PHY_INTERFACE_MODE_RXAUI)
> +		ctrl4 |= MVPP22_XLG_CTRL4_USE_XPCS;

You should probably mask MVPP22_XLG_CTRL4_USE_XPCS when the interface
isn't RXAUI (just to be consistent with what's done in the configuration
functions). You can do this a few lines before, some bits of ctrl4 get
masked.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 net-next 2/2] net: mvpp2: support multiple comphy lanes

[Matt Pelland]

mvpp 2.2 supports RXAUI, which requires two serdes lanes, and XAUI which
requires four 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 all relevant serdes lanes are correctly configured at the
appropriate times when either RXAUI or XAUI is in use.

Signed-off-by: Matt Pelland <mpelland@starry.com>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  7 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 97 ++++++++++++++-----
 2 files changed, 77 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index 256e7c796631..d74f458ca099 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 and XAUI which
+ * requires four comphy lanes. All other modes require one.
+ */
+#define MVPP22_MAX_COMPHYS		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 1a5037a398fc..100972703f60 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -1200,17 +1200,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;
+
+		ret = phy_power_on(port->comphys[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+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 phy_power_on(port->comphy);
+	return 0;
 }
 
 static void mvpp2_port_enable(struct mvpp2_port *port)
@@ -3389,7 +3412,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,
@@ -4946,7 +4971,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);
 	}
 
@@ -5037,20 +5062,18 @@ 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;
+	unsigned int ntxqs, nrxqs, ncomphys, nrequired_comphys, thread;
 	struct device_node *port_node = to_of_node(port_fwnode);
+	struct mvpp2_port_pcpu *port_pcpu;
 	netdev_features_t features;
-	struct net_device *dev;
 	struct phylink *phylink;
-	char *mac_from = "";
-	unsigned int ntxqs, nrxqs, thread;
+	struct mvpp2_port *port;
 	unsigned long flags = 0;
+	struct net_device *dev;
+	int err, i, phy_mode;
+	char *mac_from = "";
 	bool has_tx_irqs;
 	u32 id;
-	int phy_mode;
-	int err, i;
 
 	has_tx_irqs = mvpp2_port_has_irqs(priv, port_node, &flags);
 	if (!has_tx_irqs && queue_mode == MVPP2_QDIST_MULTI_MODE) {
@@ -5084,14 +5107,38 @@ 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, ncomphys = 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;
+				break;
 			}
-			comphy = NULL;
+
+			++ncomphys;
+		}
+
+		if (phy_mode == PHY_INTERFACE_MODE_XAUI)
+			nrequired_comphys = 4;
+		else if (phy_mode == PHY_INTERFACE_MODE_RXAUI)
+			nrequired_comphys = 2;
+		else
+			nrequired_comphys = 1;
+
+		if (ncomphys < nrequired_comphys) {
+			dev_err(&pdev->dev,
+				"not enough comphys to support %s\n",
+				phy_modes(phy_mode));
+			err = -EINVAL;
+			goto err_free_netdev;
 		}
 	}
 
@@ -5106,7 +5153,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);
@@ -5143,7 +5189,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);
-- 
2.21.0

Re: [PATCH v2 net-next 2/2] net: mvpp2: support multiple comphy lanes

[Antoine Tenart]

Hello Matt,

On Thu, Aug 08, 2019 at 07:06:06PM -0400, Matt Pelland wrote:
>  
>  static void mvpp2_port_enable(struct mvpp2_port *port)
> @@ -3389,7 +3412,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);

You can drop the check on the version here, mvpp22_comphy_deinit will
return 0 if no comphy was described. (You added other calls to this
function without the check, which is fine).

> @@ -5037,20 +5062,18 @@ 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;
> +	unsigned int ntxqs, nrxqs, ncomphys, nrequired_comphys, thread;
>  	struct device_node *port_node = to_of_node(port_fwnode);
> +	struct mvpp2_port_pcpu *port_pcpu;
>  	netdev_features_t features;
> -	struct net_device *dev;
>  	struct phylink *phylink;
> -	char *mac_from = "";
> -	unsigned int ntxqs, nrxqs, thread;
> +	struct mvpp2_port *port;
>  	unsigned long flags = 0;
> +	struct net_device *dev;
> +	int err, i, phy_mode;
> +	char *mac_from = "";
>  	bool has_tx_irqs;
>  	u32 id;
> -	int phy_mode;
> -	int err, i;
>  
>  	has_tx_irqs = mvpp2_port_has_irqs(priv, port_node, &flags);
>  	if (!has_tx_irqs && queue_mode == MVPP2_QDIST_MULTI_MODE) {
> @@ -5084,14 +5107,38 @@ 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, ncomphys = 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;
> +				break;
>  			}
> -			comphy = NULL;
> +
> +			++ncomphys;
> +		}
> +
> +		if (phy_mode == PHY_INTERFACE_MODE_XAUI)
> +			nrequired_comphys = 4;
> +		else if (phy_mode == PHY_INTERFACE_MODE_RXAUI)
> +			nrequired_comphys = 2;
> +		else
> +			nrequired_comphys = 1;
> +
> +		if (ncomphys < nrequired_comphys) {
> +			dev_err(&pdev->dev,
> +				"not enough comphys to support %s\n",
> +				phy_modes(phy_mode));
> +			err = -EINVAL;
> +			goto err_free_netdev;

The comphy is optional and could not be described (some SoC do not have
a driver for their comphy, and some aren't described at all). In such
cases we do rely on the bootloader/firmware configuration. Also, I'm not
sure how that would work with dynamic reconfiguration of the mode if the
n# of lanes used changes (I'm not sure that is possible though).

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 net-next 2/2] net: mvpp2: support multiple comphy lanes

[Matt Pelland]

On Fri, Aug 09, 2019 at 10:32:50AM +0200, Antoine Tenart wrote:
> Hello Matt,
> 
> On Thu, Aug 08, 2019 at 07:06:06PM -0400, Matt Pelland wrote:
> >  
> >  static void mvpp2_port_enable(struct mvpp2_port *port)
> > @@ -3389,7 +3412,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);
> 
> You can drop the check on the version here, mvpp22_comphy_deinit will
> return 0 if no comphy was described. (You added other calls to this
> function without the check, which is fine).
> 
> > @@ -5037,20 +5062,18 @@ 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;
> > +	unsigned int ntxqs, nrxqs, ncomphys, nrequired_comphys, thread;
> >  	struct device_node *port_node = to_of_node(port_fwnode);
> > +	struct mvpp2_port_pcpu *port_pcpu;
> >  	netdev_features_t features;
> > -	struct net_device *dev;
> >  	struct phylink *phylink;
> > -	char *mac_from = "";
> > -	unsigned int ntxqs, nrxqs, thread;
> > +	struct mvpp2_port *port;
> >  	unsigned long flags = 0;
> > +	struct net_device *dev;
> > +	int err, i, phy_mode;
> > +	char *mac_from = "";
> >  	bool has_tx_irqs;
> >  	u32 id;
> > -	int phy_mode;
> > -	int err, i;
> >  
> >  	has_tx_irqs = mvpp2_port_has_irqs(priv, port_node, &flags);
> >  	if (!has_tx_irqs && queue_mode == MVPP2_QDIST_MULTI_MODE) {
> > @@ -5084,14 +5107,38 @@ 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, ncomphys = 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;
> > +				break;
> >  			}
> > -			comphy = NULL;
> > +
> > +			++ncomphys;
> > +		}
> > +
> > +		if (phy_mode == PHY_INTERFACE_MODE_XAUI)
> > +			nrequired_comphys = 4;
> > +		else if (phy_mode == PHY_INTERFACE_MODE_RXAUI)
> > +			nrequired_comphys = 2;
> > +		else
> > +			nrequired_comphys = 1;
> > +
> > +		if (ncomphys < nrequired_comphys) {
> > +			dev_err(&pdev->dev,
> > +				"not enough comphys to support %s\n",
> > +				phy_modes(phy_mode));
> > +			err = -EINVAL;
> > +			goto err_free_netdev;
> 
> The comphy is optional and could not be described (some SoC do not have
> a driver for their comphy, and some aren't described at all). In such
> cases we do rely on the bootloader/firmware configuration. Also, I'm not
> sure how that would work with dynamic reconfiguration of the mode if the
> n# of lanes used changes (I'm not sure that is possible though).
> 

I'm new to this space, but, from my limited experience it seems unlikely that
some hardware configuration would require dynamically reconfiguring the number
of comphy lanes used depending on the phy mode. Unless you disagree, instead of
removing this check or making things really complicated to support this
scenario, I propose extending the conditional above to disable sanity checking
if no comphys were parsed out of the device tree. Something like:

if (ncomphys != 0 && ncomphys < nrequired_comphys)

This would cover Maxime's request for sanity checking, which I think is
valuable, while also maintaining compatibility with platforms that have no
comphy drivers or some other issue that prevents properly defining comphy nodes
in the device tree. Does that sound reasonable?

> Thanks!
> Antoine
> 
> -- 
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Thanks,
Matt

Re: [PATCH v2 net-next 2/2] net: mvpp2: support multiple comphy lanes

[Antoine Tenart]

Hello Matt,

On Fri, Aug 09, 2019 at 06:20:28PM -0400, Matt Pelland wrote:
> On Fri, Aug 09, 2019 at 10:32:50AM +0200, Antoine Tenart wrote:
> > On Thu, Aug 08, 2019 at 07:06:06PM -0400, Matt Pelland wrote:
> > > @@ -5084,14 +5107,38 @@ 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, ncomphys = 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;
> > > +				break;
> > >  			}
> > > -			comphy = NULL;
> > > +
> > > +			++ncomphys;
> > > +		}
> > > +
> > > +		if (phy_mode == PHY_INTERFACE_MODE_XAUI)
> > > +			nrequired_comphys = 4;
> > > +		else if (phy_mode == PHY_INTERFACE_MODE_RXAUI)
> > > +			nrequired_comphys = 2;
> > > +		else
> > > +			nrequired_comphys = 1;
> > > +
> > > +		if (ncomphys < nrequired_comphys) {
> > > +			dev_err(&pdev->dev,
> > > +				"not enough comphys to support %s\n",
> > > +				phy_modes(phy_mode));
> > > +			err = -EINVAL;
> > > +			goto err_free_netdev;
> > 
> > The comphy is optional and could not be described (some SoC do not have
> > a driver for their comphy, and some aren't described at all). In such
> > cases we do rely on the bootloader/firmware configuration. Also, I'm not
> > sure how that would work with dynamic reconfiguration of the mode if the
> > n# of lanes used changes (I'm not sure that is possible though).
> > 
> 
> I'm new to this space, but, from my limited experience it seems unlikely that
> some hardware configuration would require dynamically reconfiguring the number
> of comphy lanes used depending on the phy mode. Unless you disagree, instead of
> removing this check or making things really complicated to support this
> scenario, I propose extending the conditional above to disable sanity checking
> if no comphys were parsed out of the device tree. Something like:
> 
> if (ncomphys != 0 && ncomphys < nrequired_comphys)
> 
> This would cover Maxime's request for sanity checking, which I think is
> valuable, while also maintaining compatibility with platforms that have no
> comphy drivers or some other issue that prevents properly defining comphy nodes
> in the device tree. Does that sound reasonable?

That sounds good.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 net-next 0/2] net: mvpp2: Implement RXAUI Support

[Antoine Tenart]

Hello Matt,

One small comment: it seems you made a typo on davem's email address.
It's .net, not .com :)

Thanks,
Antoine

On Thu, Aug 08, 2019 at 07:06:04PM -0400, Matt Pelland wrote:
> This patch set implements support for configuring Marvell's mvpp2 hardware for
> RXAUI operation. There are two other patches necessary for this to work
> correctly that concern Marvell's cp110 comphy that were emailed to the general
> linux-kernel mailing list earlier on. I can post them here if need be. This
> patch set was successfully tested on both a Marvell Armada 7040 based platform
> as well as an Armada 8040 based platform.
> 
> Changes since v1:
> 
> - Use reverse christmas tree formatting for all modified declaration blocks.
> - Bump MVP22_MAX_COMPHYS to 4 to allow for XAUI operation.
> - Implement comphy sanity checking.
> 
> Matt Pelland (2):
>   net: mvpp2: implement RXAUI support
>   net: mvpp2: support multiple comphy lanes
> 
>  drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |   8 +-
>  .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 129 ++++++++++++++----
>  2 files changed, 110 insertions(+), 27 deletions(-)
> 
> -- 
> 2.21.0
> 

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com