Date:   Tue, 4 Apr 2023 20:05:32 +0100
From:   Daniel Golle <daniel@...rotopia.org>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, Vladimir Oltean <vladimir.oltean@....com>,
        Alexander 'lynxis' Couzens <lynxis@...0.eu>,
        Chukun Pan <amadeus@....edu.cn>,
        John Crispin <john@...ozen.org>
Subject: Re: Convention regarding SGMII in-band autonegotiation

On Tue, Apr 04, 2023 at 03:46:41PM +0100, Russell King (Oracle) wrote:
> On Tue, Apr 04, 2023 at 12:56:40PM +0100, Daniel Golle wrote:
> [...]
> Why do you think it doesn't re-enable in-band AN?
> 
> gpy_update_interface() does this when called at various speeds:
> 
> if SPEED_2500, it clears VSPEC1_SGMII_CTRL_ANEN
> 
> if SPEED_1000, SPEED_100, or SPEED_10, it sets VSPEC1_SGMII_ANEN_ANRS
>    and VSPEC1_SGMII_ANEN_ANRS is both the VSPEC1_SGMII_CTRL_ANEN and
>    VSPEC1_SGMII_CTRL_ANRS bits.
> 
> So the situation you talk about, when switching to 2500base-X,
> VSPEC1_SGMII_CTRL_ANEN will be cleared, but when switching back to
> SGMII mode, VSPEC1_SGMII_CTRL_ANEN will be set again.
> 
> To be honest, when I was reviewing the patch adding this support back
> in June 2021, that also got me, and I was wondering whether
> VSPEC1_SGMII_CTRL_ANEN was being set afterwards... it's just the
> macro naming makes it look like it doesn't. But VSPEC1_SGMII_ANEN_ANRS
> contains both ANEN and ANRS bits.

Ok, I see it also now. So at least that works somehow.

Yet switching on Cisco SGMII in-band-status (which is what
VSPEC1_SGMII_CTRL_ANEN means) deliberately in a PHY driver which is
connected to a MAC looks wrong. As we are inside a PHY driver we
obviously have access to this PHY via MDIO and could just as well use
out-of-band status.

And it cannot work in this way with MAC drivers which are expected
to only use Cisco SGMII in-band-status in case of MLO_AN_INBAND being
set.

> [...]
> > I'm afraid we will need some kind of feature flag to indicate that a
> > MAC driver is known to behave according to convention with regards to
> > SGMII in-band-status being switched off and only in that case have the
> > PHY driver do the same, and otherwhise stick with the existing
> > codepaths and potentially unknown hardware-default behavior
> > (ie. another mess like the pre_march2020 situation...)
> 
> Yes. Thankfully, it's something that, provided the PCS implementations
> are all the same, at least phylink users should be consistent and we
> don't need another flag in pl->config to indicate anything. We just
> tell phylib that we're phylink and be done with it.

Ok, so this would work in both realtek and mxl-phy phy driver
.config_init function (pseudo-code):

if (phydev->phylink &&
    !phylink_autoneg_inband(phydev->phylink->cur_link_an_mode))
        switch_off_inband_status(phydev);

If that pattern looks good to you, I will start with patches for
mxl-gpy.c and then go on with realtek.c. In case of realtek.c we
probably should also make calling genphy_soft_reset in .soft_reset
conditional on !!phydev->phylink, and that could even become a
generic helper function, as probably many other phy drivers which
currently don't set .soft_reset to genphy_soft_reset would need
that in case phylink is being used.
Anyway, step by step...

> For everything else, I think we just have to assume "let the PHY
> driver do what it does today" as the safest course of action.
> 
> As for the pre_march2020 situation, we're down to just two drivers
> that require that now:
> 
> 1) mtk_eth_soc for its RGMII mode (which, honestly, I'm prepared to
>    break at this point, because I do not believe there are *any* users
>    out there - not only have my pleas for testers for that had no
>    response, I believe the code in mk_eth_soc to be broken.)
> 
>    I am considering removing RGMII support there for implementations
>    which have MTK_GMAC1_TRGMII but _not_ MTK_TRGMII_MT7621_CLK -
>    basically the path that calls mtk_gmac0_rgmii_adjust(). I doubt
>    anyone will complain because no one seems to be using it (or
>    they are and they're ignoring my pleas for testers - in which
>    case, being three years on, they honestly get what's coming, that
>    being a regression or not.)

So that would affect MT7623, I do have this hardware for testing, the
BPi-R2 and also Frank certainly has it flying around and would probably
be available to help testing.

I may have missed your call for help there, I can test anything you
want on the BPi-R2 board with MT7623N + MT7530:
[    2.061063] mt7530 mdio-bus:00: configuring for fixed/trgmii link mode
...
[    4.939408] mtk_soc_eth 1b100000.ethernet eth0: configuring for fixed/trgmii link mode

Is that the kind of board you'd be looking for?

For the fun of it we can of course try to also change the speed of
MT7530 CPU port to something else than 1000M...

Please point me to patches to test, I can get started right away.

> 
> 2) mv88e6xxx DSA support, which needs to be converted to phylink_pcs
>    as previously stated.
> 
> I never thought it would take 3+ years to get drivers converted, but
> sadly this shows how glacially slow progress can be in mainline, and
> the more users phylink gets, the more of a problem this is likely to
> become unless we have really good interfaces into code making use of
> it.

Powered by blists - more mailing lists