[PATCH RFC net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x

[PATCH RFC net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x

[Eric Woudstra]

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.

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.

This is part of preveously rejected: net: pcs: pcs-mtk-lynxi:
use 2500Base-X without AN. But I believe this is not the part why it was
rejected.

fixes: net: pcs: add driver for MediaTek SGMII PCS

Changes to be committed:
	modified:   drivers/net/pcs/pcs-mtk-lynxi.c

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
---
 drivers/net/pcs/pcs-mtk-lynxi.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

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,
-- 
2.42.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x

[Eric Woudstra]

>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.
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x

[Eric Woudstra]

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.
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x

[Daniel Golle]

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/


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x

[Eric Woudstra]

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/
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x

[Daniel Golle]

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/
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x

[Eric Woudstra]

>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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC net-next] net: pcs: pcs-mtk-lynxi fix mtk_pcs_lynxi_get_state() for 2500base-x

[Russell King (Oracle)]

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

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel