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.ha...@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!
