linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration for SoCs without comphy
@ 2020-06-16  8:31 Sascha Hauer
  2020-06-16  8:31 ` [PATCH 2/2] net: ethernet: mvneta: Add 2500BaseX support " Sascha Hauer
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Sascha Hauer @ 2020-06-16  8:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, linux-arm-kernel, Thomas Petazzoni, kernel,
	Russell King, Sascha Hauer, Russell King

The MVNETA_SERDES_CFG register is only available on older SoCs like the
Armada XP. On newer SoCs like the Armada 38x the fields are moved to
comphy. This patch moves the writes to this register next to the comphy
initialization, so that depending on the SoC either comphy or
MVNETA_SERDES_CFG is configured.
With this we no longer write to the MVNETA_SERDES_CFG on SoCs where it
doesn't exist.

Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/net/ethernet/marvell/mvneta.c | 80 +++++++++++++++------------
 1 file changed, 44 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 51889770958d8..9933eb4577d43 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -106,6 +106,7 @@
 #define      MVNETA_TX_IN_PRGRS                  BIT(1)
 #define      MVNETA_TX_FIFO_EMPTY                BIT(8)
 #define MVNETA_RX_MIN_FRAME_SIZE                 0x247c
+/* Only exists on Armada XP and Armada 370 */
 #define MVNETA_SERDES_CFG			 0x24A0
 #define      MVNETA_SGMII_SERDES_PROTO		 0x0cc7
 #define      MVNETA_QSGMII_SERDES_PROTO		 0x0667
@@ -3514,26 +3515,55 @@ static int mvneta_setup_txqs(struct mvneta_port *pp)
 	return 0;
 }
 
-static int mvneta_comphy_init(struct mvneta_port *pp)
+static int mvneta_comphy_init(struct mvneta_port *pp, phy_interface_t interface)
 {
 	int ret;
 
-	if (!pp->comphy)
-		return 0;
-
-	ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
-			       pp->phy_interface);
+	ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET, interface);
 	if (ret)
 		return ret;
 
 	return phy_power_on(pp->comphy);
 }
 
+static int mvneta_config_interface(struct mvneta_port *pp,
+				   phy_interface_t interface)
+{
+	int ret = 0;
+
+	if (pp->comphy) {
+		if (interface == PHY_INTERFACE_MODE_SGMII ||
+		    interface == PHY_INTERFACE_MODE_1000BASEX ||
+		    interface == PHY_INTERFACE_MODE_2500BASEX) {
+			ret = mvneta_comphy_init(pp, interface);
+		}
+	} else {
+		switch (interface) {
+		case PHY_INTERFACE_MODE_QSGMII:
+			mvreg_write(pp, MVNETA_SERDES_CFG,
+				    MVNETA_QSGMII_SERDES_PROTO);
+			break;
+
+		case PHY_INTERFACE_MODE_SGMII:
+		case PHY_INTERFACE_MODE_1000BASEX:
+			mvreg_write(pp, MVNETA_SERDES_CFG,
+				    MVNETA_SGMII_SERDES_PROTO);
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	pp->phy_interface = interface;
+
+	return ret;
+}
+
 static void mvneta_start_dev(struct mvneta_port *pp)
 {
 	int cpu;
 
-	WARN_ON(mvneta_comphy_init(pp));
+	WARN_ON(mvneta_config_interface(pp, pp->phy_interface));
 
 	mvneta_max_rx_size_set(pp, pp->pkt_size);
 	mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
@@ -3907,14 +3937,10 @@ static void mvneta_mac_config(struct phylink_config *config, unsigned int mode,
 	if (state->speed == SPEED_2500)
 		new_ctrl4 |= MVNETA_GMAC4_SHORT_PREAMBLE_ENABLE;
 
-	if (pp->comphy && pp->phy_interface != state->interface &&
-	    (state->interface == PHY_INTERFACE_MODE_SGMII ||
-	     state->interface == PHY_INTERFACE_MODE_1000BASEX ||
-	     state->interface == PHY_INTERFACE_MODE_2500BASEX)) {
-		pp->phy_interface = state->interface;
-
-		WARN_ON(phy_power_off(pp->comphy));
-		WARN_ON(mvneta_comphy_init(pp));
+	if (pp->phy_interface != state->interface) {
+		if (pp->comphy)
+			WARN_ON(phy_power_off(pp->comphy));
+		WARN_ON(mvneta_config_interface(pp, state->interface));
 	}
 
 	if (new_ctrl0 != gmac_ctrl0)
@@ -4958,20 +4984,10 @@ static void mvneta_conf_mbus_windows(struct mvneta_port *pp,
 }
 
 /* Power up the port */
-static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
+static void mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
 {
 	/* MAC Cause register should be cleared */
 	mvreg_write(pp, MVNETA_UNIT_INTR_CAUSE, 0);
-
-	if (phy_mode == PHY_INTERFACE_MODE_QSGMII)
-		mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_QSGMII_SERDES_PROTO);
-	else if (phy_mode == PHY_INTERFACE_MODE_SGMII ||
-		 phy_interface_mode_is_8023z(phy_mode))
-		mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_SGMII_SERDES_PROTO);
-	else if (!phy_interface_mode_is_rgmii(phy_mode))
-		return -EINVAL;
-
-	return 0;
 }
 
 /* Device initialization routine */
@@ -5157,11 +5173,7 @@ static int mvneta_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto err_netdev;
 
-	err = mvneta_port_power_up(pp, phy_mode);
-	if (err < 0) {
-		dev_err(&pdev->dev, "can't power up port\n");
-		goto err_netdev;
-	}
+	mvneta_port_power_up(pp, phy_mode);
 
 	/* Armada3700 network controller does not support per-cpu
 	 * operation, so only single NAPI should be initialized.
@@ -5315,11 +5327,7 @@ static int mvneta_resume(struct device *device)
 		}
 	}
 	mvneta_defaults_set(pp);
-	err = mvneta_port_power_up(pp, pp->phy_interface);
-	if (err < 0) {
-		dev_err(device, "can't power up port\n");
-		return err;
-	}
+	mvneta_port_power_up(pp, pp->phy_interface);
 
 	netif_device_attach(dev);
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] net: ethernet: mvneta: Add 2500BaseX support for SoCs without comphy
  2020-06-16  8:31 [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration for SoCs without comphy Sascha Hauer
@ 2020-06-16  8:31 ` Sascha Hauer
  2020-06-19  3:00   ` David Miller
  2020-06-19  3:00 ` [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration " David Miller
  2020-06-22 23:53 ` Russell King - ARM Linux admin
  2 siblings, 1 reply; 6+ messages in thread
From: Sascha Hauer @ 2020-06-16  8:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, linux-arm-kernel, Thomas Petazzoni, kernel,
	Russell King, Sascha Hauer

The older SoCs like Armada XP support a 2500BaseX mode in the datasheets
referred to as DR-SGMII (Double rated SGMII) or HS-SGMII (High Speed
SGMII). This is an upclocked 1000BaseX mode, thus
PHY_INTERFACE_MODE_2500BASEX is the appropriate mode define for it.
adding support for it merely means writing the correct magic value into
the MVNETA_SERDES_CFG register.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/net/ethernet/marvell/mvneta.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 9933eb4577d43..23d41550edb0d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -110,6 +110,7 @@
 #define MVNETA_SERDES_CFG			 0x24A0
 #define      MVNETA_SGMII_SERDES_PROTO		 0x0cc7
 #define      MVNETA_QSGMII_SERDES_PROTO		 0x0667
+#define      MVNETA_HSGMII_SERDES_PROTO		 0x1107
 #define MVNETA_TYPE_PRIO                         0x24bc
 #define      MVNETA_FORCE_UNI                    BIT(21)
 #define MVNETA_TXQ_CMD_1                         0x24e4
@@ -3549,6 +3550,11 @@ static int mvneta_config_interface(struct mvneta_port *pp,
 			mvreg_write(pp, MVNETA_SERDES_CFG,
 				    MVNETA_SGMII_SERDES_PROTO);
 			break;
+
+		case PHY_INTERFACE_MODE_2500BASEX:
+			mvreg_write(pp, MVNETA_SERDES_CFG,
+				    MVNETA_HSGMII_SERDES_PROTO);
+			break;
 		default:
 			return -EINVAL;
 		}
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration for SoCs without comphy
  2020-06-16  8:31 [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration for SoCs without comphy Sascha Hauer
  2020-06-16  8:31 ` [PATCH 2/2] net: ethernet: mvneta: Add 2500BaseX support " Sascha Hauer
@ 2020-06-19  3:00 ` David Miller
  2020-06-22 23:53 ` Russell King - ARM Linux admin
  2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2020-06-19  3:00 UTC (permalink / raw)
  To: s.hauer
  Cc: netdev, linux-kernel, linux-arm-kernel, thomas.petazzoni, kernel,
	linux, rmk+kernel

From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Tue, 16 Jun 2020 10:31:39 +0200

> The MVNETA_SERDES_CFG register is only available on older SoCs like the
> Armada XP. On newer SoCs like the Armada 38x the fields are moved to
> comphy. This patch moves the writes to this register next to the comphy
> initialization, so that depending on the SoC either comphy or
> MVNETA_SERDES_CFG is configured.
> With this we no longer write to the MVNETA_SERDES_CFG on SoCs where it
> doesn't exist.
> 
> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Applied.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] net: ethernet: mvneta: Add 2500BaseX support for SoCs without comphy
  2020-06-16  8:31 ` [PATCH 2/2] net: ethernet: mvneta: Add 2500BaseX support " Sascha Hauer
@ 2020-06-19  3:00   ` David Miller
  0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2020-06-19  3:00 UTC (permalink / raw)
  To: s.hauer
  Cc: netdev, linux-kernel, linux-arm-kernel, thomas.petazzoni, kernel, linux

From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Tue, 16 Jun 2020 10:31:40 +0200

> The older SoCs like Armada XP support a 2500BaseX mode in the datasheets
> referred to as DR-SGMII (Double rated SGMII) or HS-SGMII (High Speed
> SGMII). This is an upclocked 1000BaseX mode, thus
> PHY_INTERFACE_MODE_2500BASEX is the appropriate mode define for it.
> adding support for it merely means writing the correct magic value into
> the MVNETA_SERDES_CFG register.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Applied.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration for SoCs without comphy
  2020-06-16  8:31 [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration for SoCs without comphy Sascha Hauer
  2020-06-16  8:31 ` [PATCH 2/2] net: ethernet: mvneta: Add 2500BaseX support " Sascha Hauer
  2020-06-19  3:00 ` [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration " David Miller
@ 2020-06-22 23:53 ` Russell King - ARM Linux admin
  2020-06-23 15:36   ` Sascha Hauer
  2 siblings, 1 reply; 6+ messages in thread
From: Russell King - ARM Linux admin @ 2020-06-22 23:53 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: netdev, linux-kernel, linux-arm-kernel, Thomas Petazzoni, kernel

On Tue, Jun 16, 2020 at 10:31:39AM +0200, Sascha Hauer wrote:
> The MVNETA_SERDES_CFG register is only available on older SoCs like the
> Armada XP. On newer SoCs like the Armada 38x the fields are moved to
> comphy. This patch moves the writes to this register next to the comphy
> initialization, so that depending on the SoC either comphy or
> MVNETA_SERDES_CFG is configured.
> With this we no longer write to the MVNETA_SERDES_CFG on SoCs where it
> doesn't exist.
> 
> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 80 +++++++++++++++------------
>  1 file changed, 44 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 51889770958d8..9933eb4577d43 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -106,6 +106,7 @@
>  #define      MVNETA_TX_IN_PRGRS                  BIT(1)
>  #define      MVNETA_TX_FIFO_EMPTY                BIT(8)
>  #define MVNETA_RX_MIN_FRAME_SIZE                 0x247c
> +/* Only exists on Armada XP and Armada 370 */
>  #define MVNETA_SERDES_CFG			 0x24A0
>  #define      MVNETA_SGMII_SERDES_PROTO		 0x0cc7
>  #define      MVNETA_QSGMII_SERDES_PROTO		 0x0667
> @@ -3514,26 +3515,55 @@ static int mvneta_setup_txqs(struct mvneta_port *pp)
>  	return 0;
>  }
>  
> -static int mvneta_comphy_init(struct mvneta_port *pp)
> +static int mvneta_comphy_init(struct mvneta_port *pp, phy_interface_t interface)
>  {
>  	int ret;
>  
> -	if (!pp->comphy)
> -		return 0;
> -
> -	ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
> -			       pp->phy_interface);
> +	ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET, interface);
>  	if (ret)
>  		return ret;
>  
>  	return phy_power_on(pp->comphy);
>  }
>  
> +static int mvneta_config_interface(struct mvneta_port *pp,
> +				   phy_interface_t interface)
> +{
> +	int ret = 0;
> +
> +	if (pp->comphy) {
> +		if (interface == PHY_INTERFACE_MODE_SGMII ||
> +		    interface == PHY_INTERFACE_MODE_1000BASEX ||
> +		    interface == PHY_INTERFACE_MODE_2500BASEX) {
> +			ret = mvneta_comphy_init(pp, interface);
> +		}
> +	} else {
> +		switch (interface) {
> +		case PHY_INTERFACE_MODE_QSGMII:
> +			mvreg_write(pp, MVNETA_SERDES_CFG,
> +				    MVNETA_QSGMII_SERDES_PROTO);
> +			break;
> +
> +		case PHY_INTERFACE_MODE_SGMII:
> +		case PHY_INTERFACE_MODE_1000BASEX:
> +			mvreg_write(pp, MVNETA_SERDES_CFG,
> +				    MVNETA_SGMII_SERDES_PROTO);
> +			break;
> +		default:
> +			return -EINVAL;

I've just noticed that you made changes to the patch I sent, such as
adding this default case that errors out, and by doing so, you have
caused a regression by causing a WARN_ON() splat.

It was not accidental that my patch had "break;" here instead of an
error return, and I left the interface mode checking in
mvneta_port_power_up() that you also removed.

mvneta supports RGMII, and since RGMII doesn't use the serdes, there
is no need to write to MVNETA_SGMII_SERDES_PROTO, and so we want to
ignore those, not return -EINVAL.

Since the interface type was already validated both by phylink when
the interface is brought up, and also by the driver at probe time
through mvneta_port_power_up(), which performs early validation of
the mode given in DT this was not a problem... there is no need to
consider anything but the RGMII case in the "default" case here.

So, please fix this... at minimum fixing this switch() statement not
to error out in the RGMII cases.  However, I think actually following
what was in my patch (which was there for good reason) rather than
randomly changing it would have been better.

This will have made the kernel on the SolidRun Clearfog platform
trigger the WARN_ON()s for the dedicated gigabit port, which uses
RGMII, and doesn't have a comphy specified in DT... and having
waited for the compile to finish and the resulting kernel to boot...

WARNING: CPU: 0 PID: 268 at drivers/net/ethernet/marvell/mvneta.c:3512 mvneta_start_dev+0x220/0x23c

Thanks.

> +		}
> +	}
> +
> +	pp->phy_interface = interface;
> +
> +	return ret;
> +}
> +
>  static void mvneta_start_dev(struct mvneta_port *pp)
>  {
>  	int cpu;
>  
> -	WARN_ON(mvneta_comphy_init(pp));
> +	WARN_ON(mvneta_config_interface(pp, pp->phy_interface));
>  
>  	mvneta_max_rx_size_set(pp, pp->pkt_size);
>  	mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
> @@ -3907,14 +3937,10 @@ static void mvneta_mac_config(struct phylink_config *config, unsigned int mode,
>  	if (state->speed == SPEED_2500)
>  		new_ctrl4 |= MVNETA_GMAC4_SHORT_PREAMBLE_ENABLE;
>  
> -	if (pp->comphy && pp->phy_interface != state->interface &&
> -	    (state->interface == PHY_INTERFACE_MODE_SGMII ||
> -	     state->interface == PHY_INTERFACE_MODE_1000BASEX ||
> -	     state->interface == PHY_INTERFACE_MODE_2500BASEX)) {
> -		pp->phy_interface = state->interface;
> -
> -		WARN_ON(phy_power_off(pp->comphy));
> -		WARN_ON(mvneta_comphy_init(pp));
> +	if (pp->phy_interface != state->interface) {
> +		if (pp->comphy)
> +			WARN_ON(phy_power_off(pp->comphy));
> +		WARN_ON(mvneta_config_interface(pp, state->interface));
>  	}
>  
>  	if (new_ctrl0 != gmac_ctrl0)
> @@ -4958,20 +4984,10 @@ static void mvneta_conf_mbus_windows(struct mvneta_port *pp,
>  }
>  
>  /* Power up the port */
> -static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
> +static void mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
>  {
>  	/* MAC Cause register should be cleared */
>  	mvreg_write(pp, MVNETA_UNIT_INTR_CAUSE, 0);
> -
> -	if (phy_mode == PHY_INTERFACE_MODE_QSGMII)
> -		mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_QSGMII_SERDES_PROTO);
> -	else if (phy_mode == PHY_INTERFACE_MODE_SGMII ||
> -		 phy_interface_mode_is_8023z(phy_mode))
> -		mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_SGMII_SERDES_PROTO);
> -	else if (!phy_interface_mode_is_rgmii(phy_mode))
> -		return -EINVAL;
> -
> -	return 0;
>  }
>  
>  /* Device initialization routine */
> @@ -5157,11 +5173,7 @@ static int mvneta_probe(struct platform_device *pdev)
>  	if (err < 0)
>  		goto err_netdev;
>  
> -	err = mvneta_port_power_up(pp, phy_mode);
> -	if (err < 0) {
> -		dev_err(&pdev->dev, "can't power up port\n");
> -		goto err_netdev;
> -	}
> +	mvneta_port_power_up(pp, phy_mode);
>  
>  	/* Armada3700 network controller does not support per-cpu
>  	 * operation, so only single NAPI should be initialized.
> @@ -5315,11 +5327,7 @@ static int mvneta_resume(struct device *device)
>  		}
>  	}
>  	mvneta_defaults_set(pp);
> -	err = mvneta_port_power_up(pp, pp->phy_interface);
> -	if (err < 0) {
> -		dev_err(device, "can't power up port\n");
> -		return err;
> -	}
> +	mvneta_port_power_up(pp, pp->phy_interface);
>  
>  	netif_device_attach(dev);
>  
> -- 
> 2.27.0
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration for SoCs without comphy
  2020-06-22 23:53 ` Russell King - ARM Linux admin
@ 2020-06-23 15:36   ` Sascha Hauer
  0 siblings, 0 replies; 6+ messages in thread
From: Sascha Hauer @ 2020-06-23 15:36 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: netdev, linux-kernel, linux-arm-kernel, Thomas Petazzoni, kernel

On Tue, Jun 23, 2020 at 12:53:40AM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Jun 16, 2020 at 10:31:39AM +0200, Sascha Hauer wrote:
> > The MVNETA_SERDES_CFG register is only available on older SoCs like the
> > Armada XP. On newer SoCs like the Armada 38x the fields are moved to
> > comphy. This patch moves the writes to this register next to the comphy
> > initialization, so that depending on the SoC either comphy or
> > MVNETA_SERDES_CFG is configured.
> > With this we no longer write to the MVNETA_SERDES_CFG on SoCs where it
> > doesn't exist.
> > 
> > Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/net/ethernet/marvell/mvneta.c | 80 +++++++++++++++------------
> >  1 file changed, 44 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > index 51889770958d8..9933eb4577d43 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -106,6 +106,7 @@
> >  #define      MVNETA_TX_IN_PRGRS                  BIT(1)
> >  #define      MVNETA_TX_FIFO_EMPTY                BIT(8)
> >  #define MVNETA_RX_MIN_FRAME_SIZE                 0x247c
> > +/* Only exists on Armada XP and Armada 370 */
> >  #define MVNETA_SERDES_CFG			 0x24A0
> >  #define      MVNETA_SGMII_SERDES_PROTO		 0x0cc7
> >  #define      MVNETA_QSGMII_SERDES_PROTO		 0x0667
> > @@ -3514,26 +3515,55 @@ static int mvneta_setup_txqs(struct mvneta_port *pp)
> >  	return 0;
> >  }
> >  
> > -static int mvneta_comphy_init(struct mvneta_port *pp)
> > +static int mvneta_comphy_init(struct mvneta_port *pp, phy_interface_t interface)
> >  {
> >  	int ret;
> >  
> > -	if (!pp->comphy)
> > -		return 0;
> > -
> > -	ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
> > -			       pp->phy_interface);
> > +	ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET, interface);
> >  	if (ret)
> >  		return ret;
> >  
> >  	return phy_power_on(pp->comphy);
> >  }
> >  
> > +static int mvneta_config_interface(struct mvneta_port *pp,
> > +				   phy_interface_t interface)
> > +{
> > +	int ret = 0;
> > +
> > +	if (pp->comphy) {
> > +		if (interface == PHY_INTERFACE_MODE_SGMII ||
> > +		    interface == PHY_INTERFACE_MODE_1000BASEX ||
> > +		    interface == PHY_INTERFACE_MODE_2500BASEX) {
> > +			ret = mvneta_comphy_init(pp, interface);
> > +		}
> > +	} else {
> > +		switch (interface) {
> > +		case PHY_INTERFACE_MODE_QSGMII:
> > +			mvreg_write(pp, MVNETA_SERDES_CFG,
> > +				    MVNETA_QSGMII_SERDES_PROTO);
> > +			break;
> > +
> > +		case PHY_INTERFACE_MODE_SGMII:
> > +		case PHY_INTERFACE_MODE_1000BASEX:
> > +			mvreg_write(pp, MVNETA_SERDES_CFG,
> > +				    MVNETA_SGMII_SERDES_PROTO);
> > +			break;
> > +		default:
> > +			return -EINVAL;
> 
> I've just noticed that you made changes to the patch I sent, such as
> adding this default case that errors out, and by doing so, you have
> caused a regression by causing a WARN_ON() splat.
> 
> It was not accidental that my patch had "break;" here instead of an
> error return, and I left the interface mode checking in
> mvneta_port_power_up() that you also removed.
> 
> mvneta supports RGMII, and since RGMII doesn't use the serdes, there
> is no need to write to MVNETA_SGMII_SERDES_PROTO, and so we want to
> ignore those, not return -EINVAL.
> 
> Since the interface type was already validated both by phylink when
> the interface is brought up, and also by the driver at probe time
> through mvneta_port_power_up(), which performs early validation of
> the mode given in DT this was not a problem... there is no need to
> consider anything but the RGMII case in the "default" case here.
> 
> So, please fix this... at minimum fixing this switch() statement not
> to error out in the RGMII cases.  However, I think actually following
> what was in my patch (which was there for good reason) rather than
> randomly changing it would have been better.
> 
> This will have made the kernel on the SolidRun Clearfog platform
> trigger the WARN_ON()s for the dedicated gigabit port, which uses
> RGMII, and doesn't have a comphy specified in DT... and having
> waited for the compile to finish and the resulting kernel to boot...

I'll send a fixup shortly

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-06-23 15:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-16  8:31 [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration for SoCs without comphy Sascha Hauer
2020-06-16  8:31 ` [PATCH 2/2] net: ethernet: mvneta: Add 2500BaseX support " Sascha Hauer
2020-06-19  3:00   ` David Miller
2020-06-19  3:00 ` [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration " David Miller
2020-06-22 23:53 ` Russell King - ARM Linux admin
2020-06-23 15:36   ` Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).