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".
*/
