Re: Re: Re: [PATCH net-next v12 08/18] net: ethernet: mtk_eth_soc: fix 1000Base-X and 2500Base-X modes

[Russell King (Oracle)]

On Sun, Mar 12, 2023 at 08:05:41PM +0000, Russell King (Oracle) wrote:
> On Sun, Mar 12, 2023 at 01:40:36PM +0100, Frank Wunderlich wrote:
> > > Gesendet: Samstag, 11. März 2023 um 21:30 Uhr
> > > Von: "Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx>
> > 
> > > On Sat, Mar 11, 2023 at 09:21:47PM +0100, Frank Wunderlich wrote:
> > > > Am 11. März 2023 21:00:20 MEZ schrieb "Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx>:
> > > > >On Sat, Mar 11, 2023 at 01:05:37PM +0100, Frank Wunderlich wrote:
> > > > 
> > > > >> i got the 2.5G copper sfps, and tried them...they work well with the v12 (including this patch), but not in v13... 
> > > > 
> > > > >> how can we add a quirk to support this?
> > > > >
> > > > >Why does it need a quirk?
> > > > 
> > > > To disable the inband-mode for this 2.5g copper
> > > > sfp. But have not found a way to set a flag which i
> > > > can grab in phylink.
> > > 
> > > We could make sfp_parse_support() set Autoneg, Pause, and Asym_Pause
> > > in "modes" at the top of that function, and then use the SFP modes
> > > quirk to clear the Autoneg bit for this SFP. Would that work for you?
> > 
> > i already tried this (without moving the autoneg/pause to sfp_parse_support):
> > 
> > static void sfp_quirk_disable_autoneg(const struct sfp_eeprom_id *id,
> > 				unsigned long *modes,
> > 				unsigned long *interfaces)
> > {
> > 	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, modes);
> > }
> > 
> > quirk was executed, but no change (no link on 2g5 sfp).
> 
> It won't have any effect on its own - because sfp_parse_support() does
> this:
> 
>         if (bus->sfp_quirk && bus->sfp_quirk->modes)
>                 bus->sfp_quirk->modes(id, modes, interfaces);
> 
>         linkmode_or(support, support, modes);
> 
>         phylink_set(support, Autoneg);
>         phylink_set(support, Pause);
>         phylink_set(support, Asym_Pause);
> 
> Which means clearing Autoneg in "modes" via the modes SFP quirk will
> have *absolutely* *no* *effect* what so ever.
> 
> The fact that you replied having *not* followed my suggestion and then
> itimiating that it doesn't work is very frustrating.
> 
> > i guess you mean moving code handling the dt-property for inband-mode in phylink_parse_mode (phylink.c) to the sfp-function (drivers/net/phy/sfp-bus.c)
> 
> No.
> 
> [rest of email cut because I can't be bothered to read it after this]
> 
> Please try what I suggested. You might find that it works.

Since describing what I wanted you to test didn't work, here's a patch
instead, based upon the quirk that you provided (which is what I'd have
written anyway). Add a "#define DEBUG" to the top of
drivers/net/phy/phylink.c in addition to applying this patch, and please
test the resulting kernel, sending me the resulting kernel messages, and
also reporting whether this works or not.

Thanks.

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index daac293e8ede..1dd50f2ca05d 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -151,6 +151,10 @@ void sfp_parse_support(struct sfp_bus *bus, const struct sfp_eeprom_id *id,
 	unsigned int br_min, br_nom, br_max;
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(modes) = { 0, };
 
+	phylink_set(modes, Autoneg);
+	phylink_set(modes, Pause);
+	phylink_set(modes, Asym_Pause);
+
 	/* Decode the bitrate information to MBd */
 	br_min = br_nom = br_max = 0;
 	if (id->base.br_nominal) {
@@ -329,10 +333,6 @@ void sfp_parse_support(struct sfp_bus *bus, const struct sfp_eeprom_id *id,
 		bus->sfp_quirk->modes(id, modes, interfaces);
 
 	linkmode_or(support, support, modes);
-
-	phylink_set(support, Autoneg);
-	phylink_set(support, Pause);
-	phylink_set(support, Asym_Pause);
 }
 EXPORT_SYMBOL_GPL(sfp_parse_support);
 
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 39e3095796d0..b054360bf636 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -360,6 +360,13 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id,
 	__set_bit(PHY_INTERFACE_MODE_2500BASEX, interfaces);
 }
 
+static void sfp_quirk_disable_autoneg(const struct sfp_eeprom_id *id,
+				      unsigned long *modes,
+				      unsigned long *interfaces)
+{
+	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, modes);
+}
+
 static void sfp_quirk_ubnt_uf_instant(const struct sfp_eeprom_id *id,
 				      unsigned long *modes,
 				      unsigned long *interfaces)
@@ -401,6 +408,7 @@ static const struct sfp_quirk sfp_quirks[] = {
 	SFP_QUIRK_M("UBNT", "UF-INSTANT", sfp_quirk_ubnt_uf_instant),
 
 	SFP_QUIRK_F("OEM", "SFP-10G-T", sfp_fixup_rollball_cc),
+	SFP_QUIRK_M("OEM", "SFP-2.5G-T", sfp_quirk_disable_autoneg),
 	SFP_QUIRK_F("OEM", "RTSFP-10", sfp_fixup_rollball_cc),
 	SFP_QUIRK_F("OEM", "RTSFP-10G", sfp_fixup_rollball_cc),
 	SFP_QUIRK_F("Turris", "RTSFP-10", sfp_fixup_rollball),

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