[PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Sascha Hauer]

The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode
called DRSGMII. Depending on the Port MAC Control Register0 PortType
setting this seems to be either an overclocked SGMII mode or 2500BaseX.

This patch adds the necessary Serdes Configuration setting for the
2.5Gbps modes. There is no phy interface mode define for overclocked
SGMII, so only 2500BaseX is handled for now.

As phy_interface_mode_is_8023z() returns true for both
PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we
explicitly test for 1000BaseX instead of using
phy_interface_mode_is_8023z() to differentiate the different
possibilities.

Fixes: da58a931f248f ("net: mvneta: Add support for 2500Mbps SGMII")
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

Changes since v1:
  - Add Fixes: tag

 drivers/net/ethernet/marvell/mvneta.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 51889770958d8..3b13048931412 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -109,6 +109,7 @@
 #define MVNETA_SERDES_CFG			 0x24A0
 #define      MVNETA_SGMII_SERDES_PROTO		 0x0cc7
 #define      MVNETA_QSGMII_SERDES_PROTO		 0x0667
+#define      MVNETA_DRSGMII_SERDES_PROTO	 0x1107
 #define MVNETA_TYPE_PRIO                         0x24bc
 #define      MVNETA_FORCE_UNI                    BIT(21)
 #define MVNETA_TXQ_CMD_1                         0x24e4
@@ -4966,8 +4967,10 @@ static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
 	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))
+		 phy_mode == PHY_INTERFACE_MODE_1000BASEX)
 		mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_SGMII_SERDES_PROTO);
+	else if (phy_mode == PHY_INTERFACE_MODE_2500BASEX)
+		mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_DRSGMII_SERDES_PROTO);
 	else if (!phy_interface_mode_is_rgmii(phy_mode))
 		return -EINVAL;
 
-- 
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Russell King - ARM Linux admin]

On Fri, Jun 12, 2020 at 10:38:47AM +0200, Sascha Hauer wrote:
> The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode
> called DRSGMII. Depending on the Port MAC Control Register0 PortType
> setting this seems to be either an overclocked SGMII mode or 2500BaseX.
> 
> This patch adds the necessary Serdes Configuration setting for the
> 2.5Gbps modes. There is no phy interface mode define for overclocked
> SGMII, so only 2500BaseX is handled for now.
> 
> As phy_interface_mode_is_8023z() returns true for both
> PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we
> explicitly test for 1000BaseX instead of using
> phy_interface_mode_is_8023z() to differentiate the different
> possibilities.
> 
> Fixes: da58a931f248f ("net: mvneta: Add support for 2500Mbps SGMII")
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

2500base-X is used today on Armada 388 and Armada 3720 platforms and
works - it is known to interoperate with Marvell PP2.2 hardware, as
well was various SFPs such as the Huawei MA5671A at 2.5Gbps.  The way
it is handled on these platforms is via the COMPHY, requesting that
the serdes is upclocked from 1.25Gbps to 3.125Gbps.

This "DRSGMII" mode is not mentioned in the functional specs for either
the Armada 388 or Armada 3720, the value you poke into the register is
not mentioned either.  As I've already requested, some information on
exactly what this "DRSGMII" is would be very useful, it can't be
"double-rate SGMII" because that would give you 2Gbps instead of 1Gbps.

So, I suspect this breaks the platforms that are known to work.

We need a proper description of what DRSGMII is before we can consider
taking any patches for it.

So, sorry, but NAK.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Russell King - ARM Linux admin]

On Fri, Jun 12, 2020 at 09:47:10AM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jun 12, 2020 at 10:38:47AM +0200, Sascha Hauer wrote:
> > The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode
> > called DRSGMII. Depending on the Port MAC Control Register0 PortType
> > setting this seems to be either an overclocked SGMII mode or 2500BaseX.
> > 
> > This patch adds the necessary Serdes Configuration setting for the
> > 2.5Gbps modes. There is no phy interface mode define for overclocked
> > SGMII, so only 2500BaseX is handled for now.
> > 
> > As phy_interface_mode_is_8023z() returns true for both
> > PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we
> > explicitly test for 1000BaseX instead of using
> > phy_interface_mode_is_8023z() to differentiate the different
> > possibilities.
> > 
> > Fixes: da58a931f248f ("net: mvneta: Add support for 2500Mbps SGMII")
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> 2500base-X is used today on Armada 388 and Armada 3720 platforms and
> works - it is known to interoperate with Marvell PP2.2 hardware, as
> well was various SFPs such as the Huawei MA5671A at 2.5Gbps.  The way
> it is handled on these platforms is via the COMPHY, requesting that
> the serdes is upclocked from 1.25Gbps to 3.125Gbps.
> 
> This "DRSGMII" mode is not mentioned in the functional specs for either
> the Armada 388 or Armada 3720, the value you poke into the register is
> not mentioned either.  As I've already requested, some information on
> exactly what this "DRSGMII" is would be very useful, it can't be
> "double-rate SGMII" because that would give you 2Gbps instead of 1Gbps.
> 
> So, I suspect this breaks the platforms that are known to work.
> 
> We need a proper description of what DRSGMII is before we can consider
> taking any patches for it.

Okay, having dug through the Armada XP, 370, 388, 3720 specs, I think
this is fine after all - but something that will help for the future
would be to document that this register does not exist on the 388 and
3720 devices (which brings up the question whether we should be writing
it there.)  The field was moved into the comphy on those devices.

So, it looks like if we have a comphy, we should not be writing this
register.

What's more, the write to MVNETA_SERDES_CFG should not be in
mvneta_port_power_up(); it's likely that XP and 370 will not work
properly with phylink.  It needs to be done in a similar location to
mvneta_comphy_init(), so that phylink can switch between 1G and 2.5G
speeds.

As you have an Armada XP system, you are best placed to test moving
that write.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Russell King - ARM Linux admin]

On Fri, Jun 12, 2020 at 11:01:15AM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jun 12, 2020 at 09:47:10AM +0100, Russell King - ARM Linux admin wrote:
> > On Fri, Jun 12, 2020 at 10:38:47AM +0200, Sascha Hauer wrote:
> > > The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode
> > > called DRSGMII. Depending on the Port MAC Control Register0 PortType
> > > setting this seems to be either an overclocked SGMII mode or 2500BaseX.
> > > 
> > > This patch adds the necessary Serdes Configuration setting for the
> > > 2.5Gbps modes. There is no phy interface mode define for overclocked
> > > SGMII, so only 2500BaseX is handled for now.
> > > 
> > > As phy_interface_mode_is_8023z() returns true for both
> > > PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we
> > > explicitly test for 1000BaseX instead of using
> > > phy_interface_mode_is_8023z() to differentiate the different
> > > possibilities.
> > > 
> > > Fixes: da58a931f248f ("net: mvneta: Add support for 2500Mbps SGMII")
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > 
> > 2500base-X is used today on Armada 388 and Armada 3720 platforms and
> > works - it is known to interoperate with Marvell PP2.2 hardware, as
> > well was various SFPs such as the Huawei MA5671A at 2.5Gbps.  The way
> > it is handled on these platforms is via the COMPHY, requesting that
> > the serdes is upclocked from 1.25Gbps to 3.125Gbps.
> > 
> > This "DRSGMII" mode is not mentioned in the functional specs for either
> > the Armada 388 or Armada 3720, the value you poke into the register is
> > not mentioned either.  As I've already requested, some information on
> > exactly what this "DRSGMII" is would be very useful, it can't be
> > "double-rate SGMII" because that would give you 2Gbps instead of 1Gbps.
> > 
> > So, I suspect this breaks the platforms that are known to work.
> > 
> > We need a proper description of what DRSGMII is before we can consider
> > taking any patches for it.
> 
> Okay, having dug through the Armada XP, 370, 388, 3720 specs, I think
> this is fine after all - but something that will help for the future
> would be to document that this register does not exist on the 388 and
> 3720 devices (which brings up the question whether we should be writing
> it there.)  The field was moved into the comphy on those devices.
> 
> So, it looks like if we have a comphy, we should not be writing this
> register.
> 
> What's more, the write to MVNETA_SERDES_CFG should not be in
> mvneta_port_power_up(); it's likely that XP and 370 will not work
> properly with phylink.  It needs to be done in a similar location to
> mvneta_comphy_init(), so that phylink can switch between 1G and 2.5G
> speeds.
> 
> As you have an Armada XP system, you are best placed to test moving
> that write.

Here's my suggestion - it won't apply to mainline or net* trees, but
gives you the idea I'm proposing:

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 9e25d608d856..17db74d61bc2 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -107,9 +107,11 @@
 #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
+#define      MVNETA_SGMII_SERDES_PROTO		 0x0cc7
+#define      MVNETA_HSGMII_SERDES_PROTO		 0x1107
 #define MVNETA_TYPE_PRIO                         0x24bc
 #define      MVNETA_FORCE_UNI                    BIT(21)
 #define MVNETA_TXQ_CMD_1                         0x24e4
@@ -3457,9 +3459,6 @@ static int mvneta_comphy_init(struct mvneta_port *pp)
 {
 	int ret;
 
-	if (!pp->comphy)
-		return 0;
-
 	ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
 			       pp->phy_interface);
 	if (ret)
@@ -3468,11 +3467,47 @@ static int mvneta_comphy_init(struct mvneta_port *pp)
 	return phy_power_on(pp->comphy);
 }
 
+static int mvneta_config_interface(struct mvneta_port *pp,i
+				   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);
+		}
+	} 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;
+
+		case PHY_INTERFACE_MODE_2500BASEX:
+			mvreg_write(pp, MVNETA_SERDES_CFG,
+				    MVNETA_HSGMII_SERDES_PROTO);
+			break;
+		}
+	}
+
+	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));
 
 	mvgmac_set_max_rx_size(&pp->gmac, pp->pkt_size);
 	mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
@@ -3702,14 +3737,9 @@ static int mvneta_pcs_config(struct phylink_config *config,
 	/* We should never see Asym_Pause set */
 	WARN_ON(phylink_test(advertising, Asym_Pause));
 
-	if (pp->comphy && pp->phy_interface != interface &&
-	    (interface == PHY_INTERFACE_MODE_SGMII ||
-	     interface == PHY_INTERFACE_MODE_1000BASEX ||
-	     interface == PHY_INTERFACE_MODE_2500BASEX)) {
-		pp->phy_interface = interface;
-
+	if (pp->phy_interface != interface) {
 		WARN_ON(phy_power_off(pp->comphy));
-		WARN_ON(mvneta_comphy_init(pp));
+		mvneta_config_interface(pp, interface);
 	}
 
 	if (want_1ms_clock) {
@@ -4794,12 +4824,10 @@ static int 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))
+	if (phy_mode != PHY_INTERFACE_MODE_QSGMII &&
+	    phy_mode != PHY_INTERFACE_MODE_SGMII &&
+	    !phy_interface_mode_is_8023z(phy_mode) &&
+	    !phy_interface_mode_is_rgmii(phy_mode))
 		return -EINVAL;
 
 	return 0;

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Russell King - ARM Linux admin]

On Fri, Jun 12, 2020 at 11:18:20AM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jun 12, 2020 at 11:01:15AM +0100, Russell King - ARM Linux admin wrote:
> > On Fri, Jun 12, 2020 at 09:47:10AM +0100, Russell King - ARM Linux admin wrote:
> > > On Fri, Jun 12, 2020 at 10:38:47AM +0200, Sascha Hauer wrote:
> > > > The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode
> > > > called DRSGMII. Depending on the Port MAC Control Register0 PortType
> > > > setting this seems to be either an overclocked SGMII mode or 2500BaseX.
> > > > 
> > > > This patch adds the necessary Serdes Configuration setting for the
> > > > 2.5Gbps modes. There is no phy interface mode define for overclocked
> > > > SGMII, so only 2500BaseX is handled for now.
> > > > 
> > > > As phy_interface_mode_is_8023z() returns true for both
> > > > PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we
> > > > explicitly test for 1000BaseX instead of using
> > > > phy_interface_mode_is_8023z() to differentiate the different
> > > > possibilities.
> > > > 
> > > > Fixes: da58a931f248f ("net: mvneta: Add support for 2500Mbps SGMII")
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > 
> > > 2500base-X is used today on Armada 388 and Armada 3720 platforms and
> > > works - it is known to interoperate with Marvell PP2.2 hardware, as
> > > well was various SFPs such as the Huawei MA5671A at 2.5Gbps.  The way
> > > it is handled on these platforms is via the COMPHY, requesting that
> > > the serdes is upclocked from 1.25Gbps to 3.125Gbps.
> > > 
> > > This "DRSGMII" mode is not mentioned in the functional specs for either
> > > the Armada 388 or Armada 3720, the value you poke into the register is
> > > not mentioned either.  As I've already requested, some information on
> > > exactly what this "DRSGMII" is would be very useful, it can't be
> > > "double-rate SGMII" because that would give you 2Gbps instead of 1Gbps.
> > > 
> > > So, I suspect this breaks the platforms that are known to work.
> > > 
> > > We need a proper description of what DRSGMII is before we can consider
> > > taking any patches for it.
> > 
> > Okay, having dug through the Armada XP, 370, 388, 3720 specs, I think
> > this is fine after all - but something that will help for the future
> > would be to document that this register does not exist on the 388 and
> > 3720 devices (which brings up the question whether we should be writing
> > it there.)  The field was moved into the comphy on those devices.
> > 
> > So, it looks like if we have a comphy, we should not be writing this
> > register.
> > 
> > What's more, the write to MVNETA_SERDES_CFG should not be in
> > mvneta_port_power_up(); it's likely that XP and 370 will not work
> > properly with phylink.  It needs to be done in a similar location to
> > mvneta_comphy_init(), so that phylink can switch between 1G and 2.5G
> > speeds.
> > 
> > As you have an Armada XP system, you are best placed to test moving
> > that write.
> 
> Here's my suggestion - it won't apply to mainline or net* trees, but
> gives you the idea I'm proposing:
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 9e25d608d856..17db74d61bc2 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -107,9 +107,11 @@
>  #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
> +#define      MVNETA_SGMII_SERDES_PROTO		 0x0cc7
> +#define      MVNETA_HSGMII_SERDES_PROTO		 0x1107
>  #define MVNETA_TYPE_PRIO                         0x24bc
>  #define      MVNETA_FORCE_UNI                    BIT(21)
>  #define MVNETA_TXQ_CMD_1                         0x24e4
> @@ -3457,9 +3459,6 @@ static int mvneta_comphy_init(struct mvneta_port *pp)
>  {
>  	int ret;
>  
> -	if (!pp->comphy)
> -		return 0;
> -
>  	ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
>  			       pp->phy_interface);
>  	if (ret)
> @@ -3468,11 +3467,47 @@ static int mvneta_comphy_init(struct mvneta_port *pp)
>  	return phy_power_on(pp->comphy);
>  }
>  
> +static int mvneta_config_interface(struct mvneta_port *pp,i
> +				   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);
> +		}
> +	} 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;
> +
> +		case PHY_INTERFACE_MODE_2500BASEX:
> +			mvreg_write(pp, MVNETA_SERDES_CFG,
> +				    MVNETA_HSGMII_SERDES_PROTO);
> +			break;
> +		}
> +	}
> +
> +	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));
>  
>  	mvgmac_set_max_rx_size(&pp->gmac, pp->pkt_size);
>  	mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
> @@ -3702,14 +3737,9 @@ static int mvneta_pcs_config(struct phylink_config *config,
>  	/* We should never see Asym_Pause set */
>  	WARN_ON(phylink_test(advertising, Asym_Pause));
>  
> -	if (pp->comphy && pp->phy_interface != interface &&
> -	    (interface == PHY_INTERFACE_MODE_SGMII ||
> -	     interface == PHY_INTERFACE_MODE_1000BASEX ||
> -	     interface == PHY_INTERFACE_MODE_2500BASEX)) {
> -		pp->phy_interface = interface;
> -
> +	if (pp->phy_interface != interface) {
>  		WARN_ON(phy_power_off(pp->comphy));
> -		WARN_ON(mvneta_comphy_init(pp));
> +		mvneta_config_interface(pp, interface);
>  	}
>  
>  	if (want_1ms_clock) {
> @@ -4794,12 +4824,10 @@ static int 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))
> +	if (phy_mode != PHY_INTERFACE_MODE_QSGMII &&
> +	    phy_mode != PHY_INTERFACE_MODE_SGMII &&
> +	    !phy_interface_mode_is_8023z(phy_mode) &&
> +	    !phy_interface_mode_is_rgmii(phy_mode))
>  		return -EINVAL;
>  
>  	return 0;

With the obvious mistakes fixed (extraneous 'i' and lack of default
case), it seems to still work on Armada 388 Clearfog Pro with 2.5G
modules.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Russell King - ARM Linux admin]

On Fri, Jun 12, 2020 at 11:42:08AM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jun 12, 2020 at 11:18:20AM +0100, Russell King - ARM Linux admin wrote:
> > On Fri, Jun 12, 2020 at 11:01:15AM +0100, Russell King - ARM Linux admin wrote:
> > > On Fri, Jun 12, 2020 at 09:47:10AM +0100, Russell King - ARM Linux admin wrote:
> > > > On Fri, Jun 12, 2020 at 10:38:47AM +0200, Sascha Hauer wrote:
> > > > > The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode
> > > > > called DRSGMII. Depending on the Port MAC Control Register0 PortType
> > > > > setting this seems to be either an overclocked SGMII mode or 2500BaseX.
> > > > > 
> > > > > This patch adds the necessary Serdes Configuration setting for the
> > > > > 2.5Gbps modes. There is no phy interface mode define for overclocked
> > > > > SGMII, so only 2500BaseX is handled for now.
> > > > > 
> > > > > As phy_interface_mode_is_8023z() returns true for both
> > > > > PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we
> > > > > explicitly test for 1000BaseX instead of using
> > > > > phy_interface_mode_is_8023z() to differentiate the different
> > > > > possibilities.
> > > > > 
> > > > > Fixes: da58a931f248f ("net: mvneta: Add support for 2500Mbps SGMII")
> > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > 
> > > > 2500base-X is used today on Armada 388 and Armada 3720 platforms and
> > > > works - it is known to interoperate with Marvell PP2.2 hardware, as
> > > > well was various SFPs such as the Huawei MA5671A at 2.5Gbps.  The way
> > > > it is handled on these platforms is via the COMPHY, requesting that
> > > > the serdes is upclocked from 1.25Gbps to 3.125Gbps.
> > > > 
> > > > This "DRSGMII" mode is not mentioned in the functional specs for either
> > > > the Armada 388 or Armada 3720, the value you poke into the register is
> > > > not mentioned either.  As I've already requested, some information on
> > > > exactly what this "DRSGMII" is would be very useful, it can't be
> > > > "double-rate SGMII" because that would give you 2Gbps instead of 1Gbps.
> > > > 
> > > > So, I suspect this breaks the platforms that are known to work.
> > > > 
> > > > We need a proper description of what DRSGMII is before we can consider
> > > > taking any patches for it.
> > > 
> > > Okay, having dug through the Armada XP, 370, 388, 3720 specs, I think
> > > this is fine after all - but something that will help for the future
> > > would be to document that this register does not exist on the 388 and
> > > 3720 devices (which brings up the question whether we should be writing
> > > it there.)  The field was moved into the comphy on those devices.
> > > 
> > > So, it looks like if we have a comphy, we should not be writing this
> > > register.
> > > 
> > > What's more, the write to MVNETA_SERDES_CFG should not be in
> > > mvneta_port_power_up(); it's likely that XP and 370 will not work
> > > properly with phylink.  It needs to be done in a similar location to
> > > mvneta_comphy_init(), so that phylink can switch between 1G and 2.5G
> > > speeds.
> > > 
> > > As you have an Armada XP system, you are best placed to test moving
> > > that write.
> > 
> > Here's my suggestion - it won't apply to mainline or net* trees, but
> > gives you the idea I'm proposing:
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > index 9e25d608d856..17db74d61bc2 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -107,9 +107,11 @@
> >  #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
> > +#define      MVNETA_SGMII_SERDES_PROTO		 0x0cc7
> > +#define      MVNETA_HSGMII_SERDES_PROTO		 0x1107
> >  #define MVNETA_TYPE_PRIO                         0x24bc
> >  #define      MVNETA_FORCE_UNI                    BIT(21)
> >  #define MVNETA_TXQ_CMD_1                         0x24e4
> > @@ -3457,9 +3459,6 @@ static int mvneta_comphy_init(struct mvneta_port *pp)
> >  {
> >  	int ret;
> >  
> > -	if (!pp->comphy)
> > -		return 0;
> > -
> >  	ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
> >  			       pp->phy_interface);
> >  	if (ret)
> > @@ -3468,11 +3467,47 @@ static int mvneta_comphy_init(struct mvneta_port *pp)
> >  	return phy_power_on(pp->comphy);
> >  }
> >  
> > +static int mvneta_config_interface(struct mvneta_port *pp,i
> > +				   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);
> > +		}
> > +	} 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;
> > +
> > +		case PHY_INTERFACE_MODE_2500BASEX:
> > +			mvreg_write(pp, MVNETA_SERDES_CFG,
> > +				    MVNETA_HSGMII_SERDES_PROTO);
> > +			break;
> > +		}
> > +	}
> > +
> > +	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));
> >  
> >  	mvgmac_set_max_rx_size(&pp->gmac, pp->pkt_size);
> >  	mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
> > @@ -3702,14 +3737,9 @@ static int mvneta_pcs_config(struct phylink_config *config,
> >  	/* We should never see Asym_Pause set */
> >  	WARN_ON(phylink_test(advertising, Asym_Pause));
> >  
> > -	if (pp->comphy && pp->phy_interface != interface &&
> > -	    (interface == PHY_INTERFACE_MODE_SGMII ||
> > -	     interface == PHY_INTERFACE_MODE_1000BASEX ||
> > -	     interface == PHY_INTERFACE_MODE_2500BASEX)) {
> > -		pp->phy_interface = interface;
> > -
> > +	if (pp->phy_interface != interface) {
> >  		WARN_ON(phy_power_off(pp->comphy));
> > -		WARN_ON(mvneta_comphy_init(pp));
> > +		mvneta_config_interface(pp, interface);
> >  	}
> >  
> >  	if (want_1ms_clock) {
> > @@ -4794,12 +4824,10 @@ static int 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))
> > +	if (phy_mode != PHY_INTERFACE_MODE_QSGMII &&
> > +	    phy_mode != PHY_INTERFACE_MODE_SGMII &&
> > +	    !phy_interface_mode_is_8023z(phy_mode) &&
> > +	    !phy_interface_mode_is_rgmii(phy_mode))
> >  		return -EINVAL;
> >  
> >  	return 0;
> 
> With the obvious mistakes fixed (extraneous 'i' and lack of default
> case), it seems to still work on Armada 388 Clearfog Pro with 2.5G
> modules.

... and the other bug fixed - mvneta_comphy_init() needs to be passed
the interface mode.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Russell King - ARM Linux admin]

On Fri, Jun 12, 2020 at 12:22:13PM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jun 12, 2020 at 11:42:08AM +0100, Russell King - ARM Linux admin wrote:
> > With the obvious mistakes fixed (extraneous 'i' and lack of default
> > case), it seems to still work on Armada 388 Clearfog Pro with 2.5G
> > modules.
> 
> ... and the other bug fixed - mvneta_comphy_init() needs to be passed
> the interface mode.

Unrelated to the patch, has anyone noticed that mvneta's performance
seems to have reduced?  I've only just noticed it (which makes 2.5Gbps
rather pointless).  This is iperf between two clearfogs with a 2.5G
fibre link:

root@clearfog21:~# iperf -V -c fe80::250:43ff:fe02:303%eno2
------------------------------------------------------------
Client connecting to fe80::250:43ff:fe02:303%eno2, TCP port 5001
TCP window size: 43.8 KByte (default)
------------------------------------------------------------
[  3] local fe80::250:43ff:fe21:203 port 48928 connected with fe80::250:43ff:fe02:303 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.0 sec   553 MBytes   464 Mbits/sec

I checked with Jon Nettleton, and he confirms my recollection that
mvneta on Armada 388 used to be able to fill a 2.5Gbps link.

If Armada 388 can't manage, then I suspect Armada XP will have worse
performance being an earlier revision SoC.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Sascha Hauer]

On Fri, Jun 12, 2020 at 12:30:31PM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jun 12, 2020 at 12:22:13PM +0100, Russell King - ARM Linux admin wrote:
> > On Fri, Jun 12, 2020 at 11:42:08AM +0100, Russell King - ARM Linux admin wrote:
> > > With the obvious mistakes fixed (extraneous 'i' and lack of default
> > > case), it seems to still work on Armada 388 Clearfog Pro with 2.5G
> > > modules.
> > 
> > ... and the other bug fixed - mvneta_comphy_init() needs to be passed
> > the interface mode.
> 
> Unrelated to the patch, has anyone noticed that mvneta's performance
> seems to have reduced?  I've only just noticed it (which makes 2.5Gbps
> rather pointless).  This is iperf between two clearfogs with a 2.5G
> fibre link:
> 
> root@clearfog21:~# iperf -V -c fe80::250:43ff:fe02:303%eno2
> ------------------------------------------------------------
> Client connecting to fe80::250:43ff:fe02:303%eno2, TCP port 5001
> TCP window size: 43.8 KByte (default)
> ------------------------------------------------------------
> [  3] local fe80::250:43ff:fe21:203 port 48928 connected with fe80::250:43ff:fe02:303 port 5001
> [ ID] Interval       Transfer     Bandwidth
> [  3]  0.0-10.0 sec   553 MBytes   464 Mbits/sec
> 
> I checked with Jon Nettleton, and he confirms my recollection that
> mvneta on Armada 388 used to be able to fill a 2.5Gbps link.
> 
> If Armada 388 can't manage, then I suspect Armada XP will have worse
> performance being an earlier revision SoC.

I only have one board with a Armada XP here which has a loopback cable
between two ports. It gives me:

[  3] local 172.16.1.4 port 47002 connected with 172.16.1.0 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.0 sec  1.27 GBytes  1.09 Gbits/sec

Still not 2.5Gbps, but at least twice the data rate you get, plus my
board has to handle both ends of the link.

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 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Russell King - ARM Linux admin]

On Fri, Jun 12, 2020 at 01:52:50PM +0200, Sascha Hauer wrote:
> On Fri, Jun 12, 2020 at 12:30:31PM +0100, Russell King - ARM Linux admin wrote:
> > On Fri, Jun 12, 2020 at 12:22:13PM +0100, Russell King - ARM Linux admin wrote:
> > > On Fri, Jun 12, 2020 at 11:42:08AM +0100, Russell King - ARM Linux admin wrote:
> > > > With the obvious mistakes fixed (extraneous 'i' and lack of default
> > > > case), it seems to still work on Armada 388 Clearfog Pro with 2.5G
> > > > modules.
> > > 
> > > ... and the other bug fixed - mvneta_comphy_init() needs to be passed
> > > the interface mode.
> > 
> > Unrelated to the patch, has anyone noticed that mvneta's performance
> > seems to have reduced?  I've only just noticed it (which makes 2.5Gbps
> > rather pointless).  This is iperf between two clearfogs with a 2.5G
> > fibre link:
> > 
> > root@clearfog21:~# iperf -V -c fe80::250:43ff:fe02:303%eno2
> > ------------------------------------------------------------
> > Client connecting to fe80::250:43ff:fe02:303%eno2, TCP port 5001
> > TCP window size: 43.8 KByte (default)
> > ------------------------------------------------------------
> > [  3] local fe80::250:43ff:fe21:203 port 48928 connected with fe80::250:43ff:fe02:303 port 5001
> > [ ID] Interval       Transfer     Bandwidth
> > [  3]  0.0-10.0 sec   553 MBytes   464 Mbits/sec
> > 
> > I checked with Jon Nettleton, and he confirms my recollection that
> > mvneta on Armada 388 used to be able to fill a 2.5Gbps link.
> > 
> > If Armada 388 can't manage, then I suspect Armada XP will have worse
> > performance being an earlier revision SoC.
> 
> I only have one board with a Armada XP here which has a loopback cable
> between two ports. It gives me:
> 
> [  3] local 172.16.1.4 port 47002 connected with 172.16.1.0 port 5001
> [ ID] Interval       Transfer     Bandwidth
> [  3]  0.0-10.0 sec  1.27 GBytes  1.09 Gbits/sec
> 
> Still not 2.5Gbps, but at least twice the data rate you get, plus my
> board has to handle both ends of the link.

It turns out I still had various locking debugs and DMA API debug
enabled:

[  3] local fe80::250:43ff:fe21:203 port 39326 connected with fe80::250:43ff:fe02:303 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.0 sec  2.70 GBytes  2.32 Gbits/sec

... which is more like it for this platform!

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Sascha Hauer]

On Fri, Jun 12, 2020 at 11:18:20AM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jun 12, 2020 at 11:01:15AM +0100, Russell King - ARM Linux admin wrote:
> > On Fri, Jun 12, 2020 at 09:47:10AM +0100, Russell King - ARM Linux admin wrote:
> > > On Fri, Jun 12, 2020 at 10:38:47AM +0200, Sascha Hauer wrote:
> > > > The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode
> > > > called DRSGMII. Depending on the Port MAC Control Register0 PortType
> > > > setting this seems to be either an overclocked SGMII mode or 2500BaseX.
> > > > 
> > > > This patch adds the necessary Serdes Configuration setting for the
> > > > 2.5Gbps modes. There is no phy interface mode define for overclocked
> > > > SGMII, so only 2500BaseX is handled for now.
> > > > 
> > > > As phy_interface_mode_is_8023z() returns true for both
> > > > PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we
> > > > explicitly test for 1000BaseX instead of using
> > > > phy_interface_mode_is_8023z() to differentiate the different
> > > > possibilities.
> > > > 
> > > > Fixes: da58a931f248f ("net: mvneta: Add support for 2500Mbps SGMII")
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > 
> > > 2500base-X is used today on Armada 388 and Armada 3720 platforms and
> > > works - it is known to interoperate with Marvell PP2.2 hardware, as
> > > well was various SFPs such as the Huawei MA5671A at 2.5Gbps.  The way
> > > it is handled on these platforms is via the COMPHY, requesting that
> > > the serdes is upclocked from 1.25Gbps to 3.125Gbps.
> > > 
> > > This "DRSGMII" mode is not mentioned in the functional specs for either
> > > the Armada 388 or Armada 3720, the value you poke into the register is
> > > not mentioned either.  As I've already requested, some information on
> > > exactly what this "DRSGMII" is would be very useful, it can't be
> > > "double-rate SGMII" because that would give you 2Gbps instead of 1Gbps.
> > > 
> > > So, I suspect this breaks the platforms that are known to work.
> > > 
> > > We need a proper description of what DRSGMII is before we can consider
> > > taking any patches for it.
> > 
> > Okay, having dug through the Armada XP, 370, 388, 3720 specs, I think
> > this is fine after all - but something that will help for the future
> > would be to document that this register does not exist on the 388 and
> > 3720 devices (which brings up the question whether we should be writing
> > it there.)  The field was moved into the comphy on those devices.
> > 
> > So, it looks like if we have a comphy, we should not be writing this
> > register.
> > 
> > What's more, the write to MVNETA_SERDES_CFG should not be in
> > mvneta_port_power_up(); it's likely that XP and 370 will not work
> > properly with phylink.  It needs to be done in a similar location to
> > mvneta_comphy_init(), so that phylink can switch between 1G and 2.5G
> > speeds.
> > 
> > As you have an Armada XP system, you are best placed to test moving
> > that write.
> 
> Here's my suggestion - it won't apply to mainline or net* trees, but
> gives you the idea I'm proposing:
> 

And here is the same patch which applies on master and the net tree.
It works as expected on my Armada XP in 2.5Gbps mode. Provided you are
happy with the patch I can send it as a formal patch on monday if by
then you haven't done that already.

Thanks for your help on that

Sascha

---------------------------------8<------------------------------

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 946925bbcb2de..a1c48efd91b17 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -106,9 +106,11 @@
 #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
+#define      MVNETA_HSGMII_SERDES_PROTO		 0x1107
 #define MVNETA_TYPE_PRIO                         0x24bc
 #define      MVNETA_FORCE_UNI                    BIT(21)
 #define MVNETA_TXQ_CMD_1                         0x24e4
@@ -3533,9 +3535,6 @@ static int mvneta_comphy_init(struct mvneta_port *pp)
 {
 	int ret;
 
-	if (!pp->comphy)
-		return 0;
-
 	ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
 			       pp->phy_interface);
 	if (ret)
@@ -3544,11 +3543,49 @@ static int mvneta_comphy_init(struct mvneta_port *pp)
 	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);
+		}
+	} 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;
+
+		case PHY_INTERFACE_MODE_2500BASEX:
+			mvreg_write(pp, MVNETA_SERDES_CFG,
+				    MVNETA_HSGMII_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);
@@ -3753,7 +3790,7 @@ static void mvneta_validate(struct phylink_config *config,
 	struct mvneta_port *pp = netdev_priv(ndev);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 
-	/* We only support QSGMII, SGMII, 802.3z and RGMII modes */
+	/* We only support QSGMII, SGMII, DRSGMII, 802.3z and RGMII modes */
 	if (state->interface != PHY_INTERFACE_MODE_NA &&
 	    state->interface != PHY_INTERFACE_MODE_QSGMII &&
 	    state->interface != PHY_INTERFACE_MODE_SGMII &&
@@ -3926,15 +3963,8 @@ 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)
+		WARN_ON(mvneta_config_interface(pp, state->interface));
 
 	if (new_ctrl0 != gmac_ctrl0)
 		mvreg_write(pp, MVNETA_GMAC_CTRL_0, new_ctrl0);
@@ -4977,20 +5007,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 */
@@ -5176,11 +5196,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.
@@ -5334,11 +5350,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

-- 
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 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Russell King - ARM Linux admin]

On Fri, Jun 12, 2020 at 02:06:04PM +0200, Sascha Hauer wrote:
> And here is the same patch which applies on master and the net tree.
> It works as expected on my Armada XP in 2.5Gbps mode. Provided you are
> happy with the patch I can send it as a formal patch on monday if by
> then you haven't done that already.

As mentioned in one of my replies, there's a bug the patch I sent...

> @@ -3533,9 +3535,6 @@ static int mvneta_comphy_init(struct mvneta_port *pp)
>  {
>  	int ret;
>  
> -	if (!pp->comphy)
> -		return 0;
> -
>  	ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
>  			       pp->phy_interface);

mvneta_comphy_init() needs to be passed the interface mode, and pass it
thrugh to phy_set_mode_ext().

>  	if (ret)
> @@ -3544,11 +3543,49 @@ static int mvneta_comphy_init(struct mvneta_port *pp)
>  	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);

and this needs to be:
			ret = mvneta_comphy_init(pp, interface);

Otherwise, the comphy uses the _old_ interface mode each time this
function is called.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes

[Sascha Hauer]

On Fri, Jun 12, 2020 at 09:47:10AM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jun 12, 2020 at 10:38:47AM +0200, Sascha Hauer wrote:
> > The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode
> > called DRSGMII. Depending on the Port MAC Control Register0 PortType
> > setting this seems to be either an overclocked SGMII mode or 2500BaseX.
> > 
> > This patch adds the necessary Serdes Configuration setting for the
> > 2.5Gbps modes. There is no phy interface mode define for overclocked
> > SGMII, so only 2500BaseX is handled for now.
> > 
> > As phy_interface_mode_is_8023z() returns true for both
> > PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we
> > explicitly test for 1000BaseX instead of using
> > phy_interface_mode_is_8023z() to differentiate the different
> > possibilities.
> > 
> > Fixes: da58a931f248f ("net: mvneta: Add support for 2500Mbps SGMII")
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> 2500base-X is used today on Armada 388 and Armada 3720 platforms and
> works - it is known to interoperate with Marvell PP2.2 hardware, as
> well was various SFPs such as the Huawei MA5671A at 2.5Gbps.  The way
> it is handled on these platforms is via the COMPHY, requesting that
> the serdes is upclocked from 1.25Gbps to 3.125Gbps.

Unfortunately the functional specs I have available for the Armada 38x
completely lack the ethernet registers, So I can't tell what has to be
done there. What about the other values that are poked into
MVNETA_SERDES_CFG? Are these documented in the Armada 388 functional
spec or are they just ignored by this hardware? I'm talking about
mvneta_port_power_up():

        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_mode == PHY_INTERFACE_MODE_1000BASEX)
                mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_SGMII_SERDES_PROTO);
        else if (!phy_interface_mode_is_rgmii(phy_mode))
                return -EINVAL;

In the Armada 38x functional specs we have this to configure the SGMII
mode:

PIN_PHY_GEN Setting:

SGMII SGMII (1.25 Gbps) 0x6
  HS-SGMII (3.125 Gbps) 0x8

The Armada XP doesn't have Comphy, so I guess what is being done in
mvneta_port_power_up() is just the old way for configuring the Serdes
lanes for different bitrates. Also they seem to have renamed DRSGMII
to HS-SGMII.

> 
> This "DRSGMII" mode is not mentioned in the functional specs for either
> the Armada 388 or Armada 3720, the value you poke into the register is
> not mentioned either.  As I've already requested, some information on
> exactly what this "DRSGMII" is would be very useful, it can't be
> "double-rate SGMII" because that would give you 2Gbps instead of 1Gbps.

As said, despite the fact that two times 1Gbps is not 2.5Gbps DRSGMII
still stands for "Double Rated-SGMII", as found in the MV78260 Hardware
specifications. Another place in the same document talks about "DRSGMII
(SGMII at 2.5Gbps)". Otherwise documentation is sparse, to my
information it is really only a higher bitrate.

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 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel