| Message ID | 20240102074408.1049203-1-ericwouds@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x | expand |
On Tue, Jan 02, 2024 at 08:44:08AM +0100, Eric Woudstra wrote: > From: Daniel Golle <daniel@makrotopia.org> > > Need to fix mtk_pcs_lynxi_get_state() in order for the pcs to function > correctly when the interface is set to 2500base-x, even when > PHYLINK_PCS_NEG_INBAND_DISABLED is set. Please describe your setup more fully. What is the link partner on this 2500base-X link? In PHYLINK_PCS_NEG_INBAND_DISABLED mode, this means that phylink is operating in inband mode, but Autoneg is clear in the advertisement mask, meaning Autoneg is disabled and we are using a "fixed" setting. state->speed and state->duplex should already be initialised. > When the pcs is set to 2500base-x, the register values are not compatible > with phylink_mii_c22_pcs_decode_state(). It results in parameters such as > speed unknown and such. Then the mac/pcs are setup incorrectly and do not > function. Since Autoneg is clear, phylink_mii_c22_pcs_decode_state() won't change state->speed and state->duplex, which should already be correctly set.
>Please describe your setup more fully. What is the link partner on this >2500base-X link? I use a BananaPi R3, with the oem-sfp2.5g-t module. It has the SFP quirk that disables autoneg. I was trying Marek's rtl8221b patchset, but found that even with unmodified code, original net-next unmodified, I could get link up, but no traffic is going through. On the other side is a.rock5b with rtl8125b. Only after applying this patch, it works and eth1 reports link up with 2.5Gbps instead of unknown speed. If you need more debugging info, I can supply it at a later time. On January 2, 2024 1:10:01 PM GMT+01:00, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: >On Tue, Jan 02, 2024 at 08:44:08AM +0100, Eric Woudstra wrote: >> From: Daniel Golle <daniel@makrotopia.org> >> >> Need to fix mtk_pcs_lynxi_get_state() in order for the pcs to function >> correctly when the interface is set to 2500base-x, even when >> PHYLINK_PCS_NEG_INBAND_DISABLED is set. > >Please describe your setup more fully. What is the link partner on this >2500base-X link? > >In PHYLINK_PCS_NEG_INBAND_DISABLED mode, this means that phylink is >operating in inband mode, but Autoneg is clear in the advertisement >mask, meaning Autoneg is disabled and we are using a "fixed" setting. >state->speed and state->duplex should already be initialised. > >> When the pcs is set to 2500base-x, the register values are not compatible >> with phylink_mii_c22_pcs_decode_state(). It results in parameters such as >> speed unknown and such. Then the mac/pcs are setup incorrectly and do not >> function. > >Since Autoneg is clear, phylink_mii_c22_pcs_decode_state() won't >change state->speed and state->duplex, which should already be >correctly set. >
With some extra info: echo "file drivers/net/phy/* +p" > /sys/kernel/debug/dynamic_debug/control The log looks like this when sfp inserted: With the path (on original net-next, no further modifications), Traffic OK: [ 71.212634] sfp sfp-1: mod-def0 0 -> 1 [ 71.216403] sfp sfp-1: tx-fault 1 -> 0 [ 71.220140] sfp sfp-1: SM: enter empty:up:down event insert [ 71.225716] sfp sfp-1: SM: exit probe:up:down [ 71.230059] sfp sfp-1: SM: enter probe:up:down event tx_clear [ 71.235803] sfp sfp-1: SM: exit probe:up:down [ 71.240195] sfp sfp-1: tx-fault 0 -> 1 [ 71.243939] sfp sfp-1: SM: enter probe:up:down event tx_fault [ 71.249688] sfp sfp-1: SM: exit probe:up:down [ 71.254052] sfp sfp-1: tx-fault 1 -> 0 [ 71.257810] sfp sfp-1: SM: enter probe:up:down event tx_clear [ 71.263542] sfp sfp-1: SM: exit probe:up:down [ 71.534808] sfp sfp-1: SM: enter probe:up:down event timeout [ 71.570662] sfp sfp-1: module OEM SFP-2.5G-T rev 1.0 sn SK2301110007 dc 230110 [ 71.580153] mtk_soc_eth 15100000.ethernet eth1: optical SFP: interfaces=[mac=2-4,22-23, sfp=23] [ 71.588848] mtk_soc_eth 15100000.ethernet eth1: interface 23 (2500base-x) rate match none supports 10,13-14,47 [ 71.598941] mtk_soc_eth 15100000.ethernet eth1: optical SFP: chosen 2500base-x interface [ 71.607605] mtk_soc_eth 15100000.ethernet eth1: requesting link mode inband/2500base-x with support 00,00000000,00008000,00006400 [ 71.619321] sfp sfp-1: tx disable 1 -> 0 [ 71.623287] sfp sfp-1: SM: exit present:up:wait [ 71.636489] hwmon hwmon0: temp1_input not attached to any thermal zone [ 71.684749] sfp sfp-1: SM: enter present:up:wait event timeout [ 71.690587] sfp sfp-1: SM: exit present:up:wait_los [ 74.704872] sfp sfp-1: los 1 -> 0 [ 74.708199] sfp sfp-1: SM: enter present:up:wait_los event los_low [ 74.714389] sfp sfp-1: SM: exit present:up:link_up [ 74.714422] mtk_soc_eth 15100000.ethernet eth1: Link is Up - 2.5Gbps/Full - flow control off Without the patch, No traffic possible: [ 261.515414] sfp sfp-1: los 1 -> 0 [ 261.518740] sfp sfp-1: SM: enter empty:up:down event los_low [ 261.524418] sfp sfp-1: SM: exit empty:up:down [ 261.528799] sfp sfp-1: mod-def0 0 -> 1 [ 261.532541] sfp sfp-1: los 0 -> 1 [ 261.535843] sfp sfp-1: SM: enter empty:up:down event insert [ 261.541406] sfp sfp-1: SM: exit probe:up:down [ 261.545748] sfp sfp-1: SM: enter probe:up:down event los_high [ 261.551481] sfp sfp-1: SM: exit probe:up:down [ 261.555859] sfp sfp-1: tx-fault 1 -> 0 [ 261.559595] sfp sfp-1: SM: enter probe:up:down event tx_clear [ 261.565330] sfp sfp-1: SM: exit probe:up:down [ 261.859940] sfp sfp-1: SM: enter probe:up:down event timeout [ 261.895690] sfp sfp-1: module OEM SFP-2.5G-T rev 1.0 sn SK2301110007 dc 230110 [ 261.905218] mtk_soc_eth 15100000.ethernet eth1: optical SFP: interfaces=[mac=2-4,22-23, sfp=23] [ 261.913929] mtk_soc_eth 15100000.ethernet eth1: interface 23 (2500base-x) rate match none supports 10,13-14,47 [ 261.924104] mtk_soc_eth 15100000.ethernet eth1: optical SFP: chosen 2500base-x interface [ 261.932199] mtk_soc_eth 15100000.ethernet eth1: requesting link mode inband/2500base-x with support 00,00000000,00008000,00006400 [ 261.945449] sfp sfp-1: tx disable 1 -> 0 [ 261.950886] sfp sfp-1: SM: exit present:up:wait [ 261.973346] hwmon hwmon0: temp1_input not attached to any thermal zone [ 262.009896] sfp sfp-1: SM: enter present:up:wait event timeout [ 262.015771] sfp sfp-1: SM: exit present:up:wait_los [ 264.842218] sfp sfp-1: los 1 -> 0 [ 264.845544] sfp sfp-1: SM: enter present:up:wait_los event los_low [ 264.851770] sfp sfp-1: SM: exit present:up:link_up [ 264.851801] mtk_soc_eth 15100000.ethernet eth1: Link is Up - Unknown/Unknown - flow control off So if phylink_mii_c22_pcs_decode_state() should not set the speed, then it is not correctly set somewhere else. On 1/2/24 13:55, Eric Woudstra wrote: >> Please describe your setup more fully. What is the link partner on this >> 2500base-X link? > > I use a BananaPi R3, with the oem-sfp2.5g-t module. It has the SFP quirk that disables autoneg. I was trying Marek's rtl8221b patchset, but found that even with unmodified code, original net-next unmodified, I could get link up, but no traffic is going through. > > On the other side is a.rock5b with rtl8125b. > > Only after applying this patch, it works and eth1 reports link up with 2.5Gbps instead of unknown speed. > > If you need more debugging info, I can supply it at a later time. > > > On January 2, 2024 1:10:01 PM GMT+01:00, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: >> On Tue, Jan 02, 2024 at 08:44:08AM +0100, Eric Woudstra wrote: >>> From: Daniel Golle <daniel@makrotopia.org> >>> >>> Need to fix mtk_pcs_lynxi_get_state() in order for the pcs to function >>> correctly when the interface is set to 2500base-x, even when >>> PHYLINK_PCS_NEG_INBAND_DISABLED is set. >> >> Please describe your setup more fully. What is the link partner on this >> 2500base-X link? >> >> In PHYLINK_PCS_NEG_INBAND_DISABLED mode, this means that phylink is >> operating in inband mode, but Autoneg is clear in the advertisement >> mask, meaning Autoneg is disabled and we are using a "fixed" setting. >> state->speed and state->duplex should already be initialised. >> >>> When the pcs is set to 2500base-x, the register values are not compatible >>> with phylink_mii_c22_pcs_decode_state(). It results in parameters such as >>> speed unknown and such. Then the mac/pcs are setup incorrectly and do not >>> function. >> >> Since Autoneg is clear, phylink_mii_c22_pcs_decode_state() won't >> change state->speed and state->duplex, which should already be >> correctly set. >>
On Tue, Jan 02, 2024 at 08:33:32PM +0100, Eric Woudstra wrote: > [...] > > So if phylink_mii_c22_pcs_decode_state() should not set the speed, then it is not correctly set somewhere else. Yes, but the fix should go to pcs-mtk-lynxi.c and you don't need to change phylink for it to work. This should be enough: https://patchwork.kernel.org/project/netdevbpf/patch/091e466912f1333bb76d23e95dc6019c9b71645f.1699565880.git.daniel@makrotopia.org/
I believe the general idea is that phylink should be aware wether to use inband or outband negotiation in order to setup the hardware correctly. Speaking of a situation where there is a PHY attached. On January 2, 2024 9:01:23 PM GMT+01:00, Daniel Golle <daniel@makrotopia.org> wrote: >On Tue, Jan 02, 2024 at 08:33:32PM +0100, Eric Woudstra wrote: >> [...] >> >> So if phylink_mii_c22_pcs_decode_state() should not set the speed, then it is not correctly set somewhere else. > >Yes, but the fix should go to pcs-mtk-lynxi.c and you don't need to >change phylink for it to work. >This should be enough: >https://patchwork.kernel.org/project/netdevbpf/patch/091e466912f1333bb76d23e95dc6019c9b71645f.1699565880.git.daniel@makrotopia.org/ >
On Tue, Jan 02, 2024 at 11:13:58PM +0100, Eric Woudstra wrote: > I believe the general idea is that phylink should be aware wether to use inband or outband negotiation in order to setup the hardware correctly. Speaking of a situation where there is a PHY attached. Well, SGMII speed/duplex AN is not defined for 2500Base-X by any standard and not supported by the hardware (unlike e.g. RealTek which came up with their own proprietary extension called HiSGMII). So we should simply never set the SGMII_SPEED_DUPLEX_AN bit if using 2500Base-X on MediaTek LynxI PCS -- in-band link status will still work, and as 2500Base-X anyway only supports 2500M speed at full duplex it is not a problem that speed and duplex are hard-coded in the driver in this case imho. And that works fine for SFPs with and without present/discoverable/accessible PHY. Surely, having phylink take care whether SGMII_SPEED_DUPLEX_AN should be set would be even nicer. I believe that source of confusion here is simply that in-band-status != SGMII_SPEED_DUPLEX_AN We *do* have in-band-status even without having SGMII_SPEED_DUPLEX_AN set with 2500Base-X link mode (as in: link being up or down and link, duplex and speed is fixed anyway for 2500Base-X). > > On January 2, 2024 9:01:23 PM GMT+01:00, Daniel Golle <daniel@makrotopia.org> wrote: > >On Tue, Jan 02, 2024 at 08:33:32PM +0100, Eric Woudstra wrote: > >> [...] > >> > >> So if phylink_mii_c22_pcs_decode_state() should not set the speed, then it is not correctly set somewhere else. > > > >Yes, but the fix should go to pcs-mtk-lynxi.c and you don't need to > >change phylink for it to work. > >This should be enough: > >https://patchwork.kernel.org/project/netdevbpf/patch/091e466912f1333bb76d23e95dc6019c9b71645f.1699565880.git.daniel@makrotopia.org/ > >
>Surely, having phylink take care whether SGMII_SPEED_DUPLEX_AN should be >set would be even nicer. > >I believe that source of confusion here is simply that > >in-band-status != SGMII_SPEED_DUPLEX_AN > >We *do* have in-band-status even without having SGMII_SPEED_DUPLEX_AN set >with 2500Base-X link mode (as in: link being up or down and link, duplex >and speed is fixed anyway for 2500Base-X). All clear, and even with autoneg disabled, one way or another, we now have a non-functional 2500base-x. Cannot manipulate anything in userland to get the connection functional. We need a fix in kernel.
On Tue, Jan 02, 2024 at 09:01:23PM +0100, Daniel Golle wrote: > On Tue, Jan 02, 2024 at 08:33:32PM +0100, Eric Woudstra wrote: > > [...] > > > > So if phylink_mii_c22_pcs_decode_state() should not set the speed, then it is not correctly set somewhere else. > > Yes, but the fix should go to pcs-mtk-lynxi.c and you don't need to > change phylink for it to work. ... which is broken thinking. It's "let's add custom hacks to drivers to implement driver specific behaviour, ignoring what is being asked of them by the upper layers."
On Wed, Jan 03, 2024 at 12:10:54AM +0100, Daniel Golle wrote: > On Tue, Jan 02, 2024 at 11:13:58PM +0100, Eric Woudstra wrote: > > I believe the general idea is that phylink should be aware wether to use inband or outband negotiation in order to setup the hardware correctly. Speaking of a situation where there is a PHY attached. > > Well, SGMII speed/duplex AN is not defined for 2500Base-X by any > standard and not supported by the hardware (unlike e.g. RealTek > which came up with their own proprietary extension called HiSGMII). I give up trying to work out whether people are abusing the SGMII term or not. SGMII *IS NOT ANY* BASE-X. SGMII is Cisco SGMII, defined to operate at 10, 100 and 1000M speeds over a single serdes lane operating at 1.25GBaud. 2500Base-X was many proprietary standards (called by many different names like HiSGMII, HS-SGMII, 2500base-X etc) that eventually got IEEE acceptance in one form as 2500base-X. ``PHY_INTERFACE_MODE_2500BASEX`` This defines a variant of 1000BASE-X which is clocked 2.5 times as fast as the 802.3 standard, giving a fixed bit rate of 3.125Gbaud. Note: not "SGMII upclocked by 2.5 times". We do have devices that _do_ use 802.3z (NOT SGMII) negotiation over 2500base-X - not for speed or duplex, but for the pause modes, and we have devices where it is specified that when operating in BASE-X mode, inband AN *must* be enabled - which means upclocking 1000base-X to 2500base-X requires inband AN for these devices. Simply hacking PCS to do whatever they care for 2500BASE-X is not acceptable. We need a *proper* solution to this, and we need to stop fart arsing about over this, and we need to stop fart arsing around calling things stuff that they are not, perpetuating the confusion in the wider industry.
diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c index 8501dd365279..dd0a1e0dbbc7 100644 --- a/drivers/net/pcs/pcs-mtk-lynxi.c +++ b/drivers/net/pcs/pcs-mtk-lynxi.c @@ -92,14 +92,23 @@ static void mtk_pcs_lynxi_get_state(struct phylink_pcs *pcs, struct phylink_link_state *state) { struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs); - unsigned int bm, adv; + unsigned int bm, bmsr, adv; /* Read the BMSR and LPA */ regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &bm); - regmap_read(mpcs->regmap, SGMSYS_PCS_ADVERTISE, &adv); + bmsr = FIELD_GET(SGMII_BMSR, bm); + + if (state->interface == PHY_INTERFACE_MODE_2500BASEX) { + state->link = !!(bmsr & BMSR_LSTATUS); + state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE); + state->speed = SPEED_2500; + state->duplex = DUPLEX_FULL; + + return; + } - phylink_mii_c22_pcs_decode_state(state, FIELD_GET(SGMII_BMSR, bm), - FIELD_GET(SGMII_LPA, adv)); + regmap_read(mpcs->regmap, SGMSYS_PCS_ADVERTISE, &adv); + phylink_mii_c22_pcs_decode_state(state, bmsr, FIELD_GET(SGMII_LPA, adv)); } static int mtk_pcs_lynxi_config(struct phylink_pcs *pcs, unsigned int neg_mode,