Messages in this thread Patch in this message | | | Date | Fri, 12 Jun 2020 11:18:20 +0100 | From | Russell King - ARM Linux admin <> | Subject | Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes |
| |
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!
| |