On Wed, Apr 24, 2024 at 07:13:35PM +0100, Russell King (Oracle) wrote: > On Wed, Apr 24, 2024 at 05:56:47PM +0200, Stefan Eichenberger wrote: > > On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote: > > > On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote: > > > > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote: > > > > > I also checked the datasheet and you are right about the 1000base-X mode > > > > > and in-band AN. What worked for us so far was to use SGMII mode even for > > > > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think > > > > > this works because as you wrote, the genphy just multiplies the clock by > > > > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we > > > > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps > > > > > and then just disable it for 2.5Gbps. I need to test it, but I have hope > > > > > that this should work. > > > > > > > > There is another way we could address this. If the querying support > > > > had a means to identify that the endpoint supports bypass mode, we > > > > could then have phylink identify that, and arrange to program the > > > > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would > > > > mean it wouldn't require the inband 16-bit word to be present. > > > > > > > > I haven't fully thought it through yet - for example, I haven't > > > > considered how we should indicate to the PCS that AN bypass mode > > > > should be enabled or disabled via the pcs_config() method. > > > > > > Okay, I've been trying to put more effort into this, but it's been slow > > > progress (sorry). > > > > > > My thoughts from a design point of view were that we could just switch > > > to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and > > > everything at the PCS layer should be able to cope, but this is not the > > > case, especially with mvneta/mvpp2. > > > > > > The problem is that mvneta/mvpp2 (and probably more) expect that > > > > > > 1) MLO_AN_INBAND means that the PCS will be using inband, and that > > > means the link up/down state won't be forced. This basically implies > > > that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the > > > PCS. > > > > > > 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and > > > that means that the link needs to be forced (since there's no way > > > for the hardware to know whether the link should be up or down.) > > > It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be > > > used for the PCS. > > > > > > So, attempting to put a resolution of the PHY and PCS abilities into > > > phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_* > > > mode alone just doesn't work. Yet... we need to do that in there when > > > considering whether inband can be enabled or not for non-PHY links. > > > > > > Basically, it needs a re-think how to solve this... > > > > Today I was playing around with my combination of mxl-gpy and mvpp2 and > > I got it working again with your patches applied. However, I hacked the > > phylink driver to only rely on what the phy and pcs support. I know this > > is not a proper solution, but it allowed me to verify the other changes. > > My idea was if the phy and pcs support inband then use it, otherwise use > > outband and ignore the rest. > > > > Here is how my minimal phylink_pcs_neg_mode test function looks like: > > > > static unsigned int phylink_pcs_neg_mode(struct phylink *pl, > > struct phylink_pcs *pcs, > > unsigned int mode, > > phy_interface_t interface, > > const unsigned long *advertising) > > { > > unsigned int phy_link_mode = 0; > > unsigned int pcs_link_mode; > > > > pcs_link_mode = phylink_pcs_query_inband(pcs, interface); > > if (pl->phydev) > > phy_link_mode = phy_query_inband(pl->phydev, interface); > > > > /* If the PCS or PHY can not provide inband, then use > > * outband. > > */ > > if (!(pcs_link_mode & LINK_INBAND_VALID) || > > !(phy_link_mode & LINK_INBAND_VALID)) > > return PHYLINK_PCS_NEG_OUTBAND; > > > > return PHYLINK_PCS_NEG_INBAND_ENABLED; > > } > > Note that I've changed the flags that get reported to be disable (bit 0)/ > enable (bit 1) rather than valid/possible/required because the former > makes the resolution easier. > > The problem is that merely returning outband doesn't cause mvneta/mvpp2 > to force the link up. So for example, here's a SFP module which doesn't > support any inband for 2500base-X nor SGMII: > > mvneta f1034000.ethernet eno2: copper SFP: interfaces=[mac=4,9-12,19,22-23, sfp= > 4,23,27] > mvneta f1034000.ethernet eno2: copper SFP: chosen 2500base-x interface > mvneta f1034000.ethernet eno2: PHY i2c:sfp:16 uses interfaces 4,23,27, validatin > g 4,23 > mvneta f1034000.ethernet eno2: interface 4 (sgmii) rate match none supports 2-3 > ,5-6,13 > mvneta f1034000.ethernet eno2: interface 23 (2500base-x) rate match none suppor > ts 6,13,47 > mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq= > POLL) > mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,000 > 08000,0000206c advertising 00,00000000,00008000,0000206c > mvneta f1034000.ethernet eno2: copper SFP: PHY link in-band modes 0x1 > mvneta f1034000.ethernet eno2: major config 2500base-x > mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01 > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none a > dv=00,00000000,00008000,0000206c pause=04 > mvneta f1034000.ethernet eno2: phylink_sfp_module_start() > mvneta f1034000.ethernet eno2: phylink_sfp_link_up() > mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off > mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off > mvneta f1034000.ethernet eno2: major config sgmii > mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01 > mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/sgmii/none adv=00,00000000,00008000,0000206c pause=00 > mvneta f1034000.ethernet eno2: pcs link down > mvneta f1034000.ethernet eno2: pcs link down > mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active > mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us > mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off > > This looks like the link is up, but it isn't - note "pcs link down". > If we look at the value of the GMAC AN status register: > > Value at address 0xf1036c10: 0x0000600a > > which indicates that the link is down, so no packets will pass. What I changed in mvpp2 is to allow turing off inband in 2500base-x. The mvpp2 driver can handle this use case in pcs_config, it will turn off AN and force the link up. I think it should also work for mvneta, at least the code looks almost the same. I get the following for the Port Auto-Negotiation Configuration Register: For 1Gbit/s it switches to SGMII and enables inband AN: Memory mapped at address 0xffffa0112000. Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6 For 2.5Gbit/s it disables inband AN and forces the link to be up: Memory mapped at address 0xffffaff88000. Value at address 0xF2132E0C (0xffffaff88e0c): 0x9042 Then the status register shows also link up for 2.5Gbit/s: Memory mapped at address 0xffffa5fe2000. Value at address 0xF2132E10 (0xffffa5fe2e10): 0x683B What might be confusing is that the port type, bit 1 in MAC Control Register0, is set to SGMII for 2.5Gbit/s, because we can only turn off autonegotiation for SGMII: Memory mapped at address 0xffff8c26c000. Value at address 0xF2132E00 (0xffff8c26ce00): 0x8BFD My example patch still uses the old macros: diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 97e38f61ac65..15fadfd61313 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -6223,9 +6223,12 @@ static unsigned int mvpp2_gmac_pcs_query_inband(struct phylink_pcs *pcs, * When <PortType> = 1 (1000BASE-X) this field must be set to 1. * Therefore, inband is "required". */ - if (phy_interface_mode_is_8023z(interface)) + if (interface == PHY_INTERFACE_MODE_1000BASEX) return LINK_INBAND_VALID | LINK_INBAND_REQUIRED; + if (interface == PHY_INTERFACE_MODE_2500BASEX) + return LINK_INBAND_VALID | LINK_INBAND_POSSIBLE; + /* SGMII and RGMII can be configured to use inband signalling of the * AN result. Indicate these as "possible". */