| Message ID | 20240102074326.1049179-1-ericwouds@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: phylink: add quirk for disabling in-band-status for mediatek pcs at 2500base-x | expand |
On Tue, Jan 02, 2024 at 08:43:26AM +0100, Eric Woudstra wrote: > In follow up to: net: pcs: pcs-mtk-lynxi: use 2500Base-X without AN > > MediaTek LynxI PCS, 2500Base-X will only work without inband status due to > hardware limitation. > > I understand this patch probably will not get approved as it is now, but > perhaps with some pointers in the correct direction to follow, I can change > it so it could be. It does however get the result that the rtl8221b on a > sfp module functions correctly, with and without (as optical sfp) the phy > attached and without using a quirk/ethtool to disable auto-negotiation. > > Introduce bool phylink_major_no_inband(pl,interface), a function similar to > bool phylink_phy_no_inband(phy). An option could be to use a function like > bool pcs->ops->supports_inband(interface), where if the function-pointer is > null, it means it is supported for all. This instead of using > of_device_is_compatible() inside the phylink_major_no_inband() function. > > Code added to phylink_major_config(): > > When there is no PHY attached, pl->pcs_neg_mode is set to > PHYLINK_PCS_NEG_INBAND_DISABLED. > > When there is a PHY attached, pl->cur_link_an_mode is set to MLO_AN_PHY. > To have the pcs function correctly with the rtl8221b, we need to do the > following to the in-band-status: > > We need to disable it when interface of the pcs is set to 2500base-x, > but need it enable it when switched to sgmii. > > So we get: > > [...] mtk_soc_eth ... eth1: phy link up sgmii/1Gbps/Full/none/rx/tx > [...] mtk_soc_eth ... eth1: phylink_mac_config: mode=inband/sgmii/none > adv=00,00000000,00000000,00000000 pause=03 > > [...] mtk_soc_eth ... eth1: phy link up 2500base-x/2.5Gbps/Full/none/rx/tx > [...] mtk_soc_eth ... eth1: phylink_mac_config: mode=phy/2500base-x/none > adv=00,00000000,00008000,0000606f pause=03 > > Changes to be committed: > modified: drivers/net/phy/phylink.c > > Signed-off-by: Eric Woudstra <ericwouds@gmail.com> > --- > drivers/net/phy/phylink.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) Your changes should go into mtk_eth_soc.c, you sould not need to modify phylink for this and as the link being up or down is still reported correctly by the hardware, it is also ok to have phylink believe that in-band-status is being used and just not set the SGMII_AN bit in the MediaTek LynxI hardware. (ie. only auto-negotiation of speed and duplex doesn't work in 2500Base-X mode) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 298dfd6982a5..6e443eb8ee46 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -1074,6 +1074,22 @@ static void phylink_pcs_an_restart(struct phylink *pl) > pl->pcs->ops->pcs_an_restart(pl->pcs); > } > > +static bool phylink_major_no_inband(struct phylink *pl, phy_interface_t interface) > +{ > + struct device_node *node = pl->config->dev->of_node; > + > + if (!node) > + return false; > + > + if (!of_device_is_compatible(node, "mediatek,eth-mac")) > + return false; > + > + if (interface != PHY_INTERFACE_MODE_2500BASEX) > + return false; > + > + return true; > +} > + > static void phylink_major_config(struct phylink *pl, bool restart, > const struct phylink_link_state *state) > { > @@ -1085,10 +1101,22 @@ static void phylink_major_config(struct phylink *pl, bool restart, > > phylink_dbg(pl, "major config %s\n", phy_modes(state->interface)); > > + if (phylink_major_no_inband(pl, state->interface) && (!!pl->phydev)) { > + if (pl->cur_link_an_mode == MLO_AN_INBAND) > + pl->cur_link_an_mode = MLO_AN_PHY; > + else > + /* restore mode if it was changed before */ > + pl->cur_link_an_mode = pl->cfg_link_an_mode; > + } > + > pl->pcs_neg_mode = phylink_pcs_neg_mode(pl->cur_link_an_mode, > state->interface, > state->advertising); > > + if (phylink_major_no_inband(pl, state->interface) && !pl->phydev && > + pl->pcs_neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) > + pl->pcs_neg_mode = PHYLINK_PCS_NEG_INBAND_DISABLED; > + > if (pl->using_mac_select_pcs) { > pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface); > if (IS_ERR(pcs)) { > @@ -1218,6 +1246,9 @@ static void phylink_mac_pcs_get_state(struct phylink *pl, > struct phylink_link_state *state) > { > linkmode_copy(state->advertising, pl->link_config.advertising); > + if (pl->pcs_neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED) > + linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, > + state->advertising); > linkmode_zero(state->lp_advertising); > state->interface = pl->link_config.interface; > state->rate_matching = pl->link_config.rate_matching; > -- > 2.42.1 >
Sorry, I'm on Android for now and have trouble find a suitable cliënt. This should have no http.
Anyway, I see now, I had another version which I should have send:
+ if (phylink_major_no_inband(pl, state->interface) && (!!pl->phydev)) {
+ if (pl->cur_link_an_mode == MLO_AN_INBAND)
+ pl->cur_link_an_mode = MLO_AN_PHY;
+ }
+ else
+ /* restore mode if it was changed before */
+ pl->cur_link_an_mode = pl->cfg_link_an_mode;
To prevent it from toggling all the time. So I do need to spend some more
attention to this part, cause this also may not be 100% ok, if changing
phylink core would be done.
The reason I do it in phylink, because of changing to MLO_AN_PHY when
there is a PHY attached. mtk_eth_soc would not. Be aware of PHY attached.
So that's why I've opened the rfc.
See which way to go with this in an acceptable way, preferably using MLO_AN_PHY.
On Tue, Jan 02, 2024 at 08:43:26AM +0100, Eric Woudstra wrote: > In follow up to: net: pcs: pcs-mtk-lynxi: use 2500Base-X without AN > > MediaTek LynxI PCS, 2500Base-X will only work without inband status due to > hardware limitation. Yes, we need better support for 2500base-X connected to a PHY. Currently, we treat both base-X interface modes as _media_ side modes by looking at the Autoneg bit. However, given that 2500base-X has been around for ages in vendor-specific forms, we need to do better. The introduction of the PHYLINK_PCS_NEG_* enum and phylink_pcs_neg_mode() is a step towards resolving this (as well as ensuring consistency of implementation so we _can_ start to address it in phylink rather than having different PCS drivers doing weird stuff.) I am _not_ of the opinion that we should be dealing with this based on compatibles or anything like that. As 6.6 was declared LTS, I think we can now move phylink_pcs_neg_mode() into phylink.c, and thus think about what we should do with: + /* 1000base-X is designed for use media-side for Fibre + * connections, and thus the Autoneg bit needs to be + * taken into account. We also do this for 2500base-X + * as well, but drivers may not support this, so may + * need to override this. + */ + if (!phylink_autoneg_inband(mode)) + neg_mode = PHYLINK_PCS_NEG_OUTBAND; + else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, + advertising)) + neg_mode = PHYLINK_PCS_NEG_INBAND_ENABLED; + else + neg_mode = PHYLINK_PCS_NEG_INBAND_DISABLED; + break; Specifically, the linkmode_test_bit() bit. When there is a PHY present, the link between the PCS and PHY should _not_ depend on Autoneg as that is indicates what we want for the _media_ side, and in the case of a PCS-to-PHY link, that is not the media. The next issue is the one that you refer to, and there's several issues: 1. some implementations of 2500base-X do not support inband 2. some implementations of 2500base-X appear to require inband (e.g. mvneta, mvpp2) I have some thoughts on this, but I don't have the time to express them in email at the moment (too much to get through post-Christmas post- Covid.)
Since using compatible is not an option and in and 2500base-x is hardware dependant, then I was thinking this would be useful. bool pcs->ops->supports_inband(interface) I'll be happy to invest some time in this, but I'll need to know which way to go, before I invest more time in something that would not be accepted.
On 1/2/24 13:36, Russell King (Oracle) wrote: > As 6.6 was declared LTS, I think we can now move phylink_pcs_neg_mode() > into phylink.c, and thus think about what we should do with: In the other rfc patch Russell King (Oracle) wrote: > Since Autoneg is clear, phylink_mii_c22_pcs_decode_state() won't > change state->speed and state->duplex, which should already be > correctly set. So the rfc patch now I have changed it to the following. Sure still not ready for the real patch, but a few steps closer. The rtl8221b can now be used as optical sfp (even without disabling autoneg in the sfp-quirk). Also it can be used with marek's rtl8221b rollball patch, that changes interface between 2500base-x and sgmii. This is tested on the BananaPi R3. The speed and duplex get are set from phylink. I'm not sure what to do with pl->link_config.pause. For now I set it to MLO_PAUSE_NONE. diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 298dfd6982a5..3f03af290fa3 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1074,6 +1055,108 @@ static void phylink_pcs_an_restart(struct phylink *pl) pl->pcs->ops->pcs_an_restart(pl->pcs); } +/* This function needs to be changed, not using compatible */ +static bool phylink_basex_no_inband(struct phylink *pl) +{ + struct device_node *node = pl->config->dev->of_node; + + if (!node) + return false; + + if (!of_device_is_compatible(node, "mediatek,eth-mac")) + return false; + + return true; +} + +static void phylink_pcs_neg_mode(struct phylink *pl, + phy_interface_t interface, + const unsigned long *advertising){ + if ((!!pl->phydev) && phylink_basex_no_inband(pl)) { + switch (interface) { + case PHY_INTERFACE_MODE_1000BASEX: + case PHY_INTERFACE_MODE_2500BASEX: + if (pl->cur_link_an_mode == MLO_AN_INBAND) + pl->cur_link_an_mode = MLO_AN_PHY; + break; + default: + /* restore mode if it was changed before */ + if ((pl->cur_link_an_mode == MLO_AN_PHY) && + (pl->cfg_link_an_mode == MLO_AN_INBAND)) + pl->cur_link_an_mode = pl->cfg_link_an_mode; + } + } + + switch (interface) { + case PHY_INTERFACE_MODE_SGMII: + case PHY_INTERFACE_MODE_QSGMII: + case PHY_INTERFACE_MODE_QUSGMII: + case PHY_INTERFACE_MODE_USXGMII: + /* These protocols are designed for use with a PHY which + * communicates its negotiation result back to the MAC via + * inband communication. Note: there exist PHYs that run + * with SGMII but do not send the inband data. + */ + if (!phylink_autoneg_inband(pl->cur_link_an_mode)) + pl->pcs_neg_mode = PHYLINK_PCS_NEG_OUTBAND; + else + pl->pcs_neg_mode = PHYLINK_PCS_NEG_INBAND_ENABLED; + break; + + case PHY_INTERFACE_MODE_1000BASEX: + case PHY_INTERFACE_MODE_2500BASEX: + /* 1000base-X is designed for use media-side for Fibre + * connections, and thus the Autoneg bit needs to be + * taken into account. We also do this for 2500base-X + * as well, but drivers may not support this, so may + * need to override this. + */ + if (!phylink_autoneg_inband(pl->cur_link_an_mode)) + pl->pcs_neg_mode = PHYLINK_PCS_NEG_OUTBAND; + else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, + advertising) && + !phylink_basex_no_inband(pl)) + pl->pcs_neg_mode = PHYLINK_PCS_NEG_INBAND_ENABLED; + else { + pl->pcs_neg_mode = PHYLINK_PCS_NEG_INBAND_DISABLED; + linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, + pl->link_config.advertising); + pl->link_config.speed = (interface == + PHY_INTERFACE_MODE_1000BASEX) ? + SPEED_1000 : SPEED_2500; + pl->link_config.duplex = DUPLEX_FULL; + pl->link_config.pause = MLO_PAUSE_NONE; /* ????? */ + + } + break; + + default: + pl->pcs_neg_mode = PHYLINK_PCS_NEG_NONE; + break; + } + + return; +} + static void phylink_major_config(struct phylink *pl, bool restart, const struct phylink_link_state *state) { @@ -1085,9 +1187,7 @@ static void phylink_major_config(struct phylink *pl, bool restart, phylink_dbg(pl, "major config %s\n", phy_modes(state->interface)); - pl->pcs_neg_mode = phylink_pcs_neg_mode(pl->cur_link_an_mode, - state->interface, - state->advertising); + phylink_pcs_neg_mode(pl, state->interface, state->advertising); if (pl->using_mac_select_pcs) { pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface); @@ -1191,9 +1291,9 @@ static int phylink_change_inband_advert(struct phylink *pl) pl->link_config.pause); /* Recompute the PCS neg mode */ - pl->pcs_neg_mode = phylink_pcs_neg_mode(pl->cur_link_an_mode, - pl->link_config.interface, - pl->link_config.advertising); + phylink_pcs_neg_mode(pl, pl->link_config.interface, + pl->link_config.advertising); + neg_mode = pl->cur_link_an_mode; if (pl->pcs->neg_mode) @@ -1222,7 +1322,8 @@ static void phylink_mac_pcs_get_state(struct phylink *pl, state->interface = pl->link_config.interface; state->rate_matching = pl->link_config.rate_matching; if (linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, - state->advertising)) { + state->advertising) && + (pl->pcs_neg_mode != PHYLINK_PCS_NEG_INBAND_DISABLED)) { state->speed = SPEED_UNKNOWN; state->duplex = DUPLEX_UNKNOWN; state->pause = MLO_PAUSE_NONE;
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 298dfd6982a5..6e443eb8ee46 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1074,6 +1074,22 @@ static void phylink_pcs_an_restart(struct phylink *pl) pl->pcs->ops->pcs_an_restart(pl->pcs); } +static bool phylink_major_no_inband(struct phylink *pl, phy_interface_t interface) +{ + struct device_node *node = pl->config->dev->of_node; + + if (!node) + return false; + + if (!of_device_is_compatible(node, "mediatek,eth-mac")) + return false; + + if (interface != PHY_INTERFACE_MODE_2500BASEX) + return false; + + return true; +} + static void phylink_major_config(struct phylink *pl, bool restart, const struct phylink_link_state *state) { @@ -1085,10 +1101,22 @@ static void phylink_major_config(struct phylink *pl, bool restart, phylink_dbg(pl, "major config %s\n", phy_modes(state->interface)); + if (phylink_major_no_inband(pl, state->interface) && (!!pl->phydev)) { + if (pl->cur_link_an_mode == MLO_AN_INBAND) + pl->cur_link_an_mode = MLO_AN_PHY; + else + /* restore mode if it was changed before */ + pl->cur_link_an_mode = pl->cfg_link_an_mode; + } + pl->pcs_neg_mode = phylink_pcs_neg_mode(pl->cur_link_an_mode, state->interface, state->advertising); + if (phylink_major_no_inband(pl, state->interface) && !pl->phydev && + pl->pcs_neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) + pl->pcs_neg_mode = PHYLINK_PCS_NEG_INBAND_DISABLED; + if (pl->using_mac_select_pcs) { pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface); if (IS_ERR(pcs)) { @@ -1218,6 +1246,9 @@ static void phylink_mac_pcs_get_state(struct phylink *pl, struct phylink_link_state *state) { linkmode_copy(state->advertising, pl->link_config.advertising); + if (pl->pcs_neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED) + linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, + state->advertising); linkmode_zero(state->lp_advertising); state->interface = pl->link_config.interface; state->rate_matching = pl->link_config.rate_matching;
In follow up to: net: pcs: pcs-mtk-lynxi: use 2500Base-X without AN MediaTek LynxI PCS, 2500Base-X will only work without inband status due to hardware limitation. I understand this patch probably will not get approved as it is now, but perhaps with some pointers in the correct direction to follow, I can change it so it could be. It does however get the result that the rtl8221b on a sfp module functions correctly, with and without (as optical sfp) the phy attached and without using a quirk/ethtool to disable auto-negotiation. Introduce bool phylink_major_no_inband(pl,interface), a function similar to bool phylink_phy_no_inband(phy). An option could be to use a function like bool pcs->ops->supports_inband(interface), where if the function-pointer is null, it means it is supported for all. This instead of using of_device_is_compatible() inside the phylink_major_no_inband() function. Code added to phylink_major_config(): When there is no PHY attached, pl->pcs_neg_mode is set to PHYLINK_PCS_NEG_INBAND_DISABLED. When there is a PHY attached, pl->cur_link_an_mode is set to MLO_AN_PHY. To have the pcs function correctly with the rtl8221b, we need to do the following to the in-band-status: We need to disable it when interface of the pcs is set to 2500base-x, but need it enable it when switched to sgmii. So we get: [...] mtk_soc_eth ... eth1: phy link up sgmii/1Gbps/Full/none/rx/tx [...] mtk_soc_eth ... eth1: phylink_mac_config: mode=inband/sgmii/none adv=00,00000000,00000000,00000000 pause=03 [...] mtk_soc_eth ... eth1: phy link up 2500base-x/2.5Gbps/Full/none/rx/tx [...] mtk_soc_eth ... eth1: phylink_mac_config: mode=phy/2500base-x/none adv=00,00000000,00008000,0000606f pause=03 Changes to be committed: modified: drivers/net/phy/phylink.c Signed-off-by: Eric Woudstra <ericwouds@gmail.com> --- drivers/net/phy/phylink.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)