Convert the at803x driver to use the generic phylib SFP handling, via a
dedicated .attach_port() callback, populating the supported interfaces.

As these devices are limited to 1000BaseX, a workaround is used to also
support, in a very limited way, copper modules. This is done by
supporting SGMII but limiting it to 1G full duplex (in which case it's
somwhat compatible with 1000BaseX).

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/qcom/at803x.c | 64 ++++++++++-------------------------
 1 file changed, 17 insertions(+), 47 deletions(-)

diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
index 51a132242462..526c88051487 100644
--- a/drivers/net/phy/qcom/at803x.c
+++ b/drivers/net/phy/qcom/at803x.c
@@ -20,7 +20,7 @@
 #include <linux/of.h>
 #include <linux/phylink.h>
 #include <linux/reset.h>
-#include <linux/sfp.h>
+#include <linux/phy_port.h>
 #include <dt-bindings/net/qca-ar803x.h>
 
 #include "qcom.h"
@@ -769,58 +769,28 @@ static int at8031_register_regulators(struct phy_device *phydev)
 	return 0;
 }
 
-static int at8031_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
+static int at8031_attach_port(struct phy_device *phydev, struct phy_port *port)
 {
-	struct phy_device *phydev = upstream;
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_support);
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
-	DECLARE_PHY_INTERFACE_MASK(interfaces);
-	phy_interface_t iface;
-
-	linkmode_zero(phy_support);
-	phylink_set(phy_support, 1000baseX_Full);
-	phylink_set(phy_support, 1000baseT_Full);
-	phylink_set(phy_support, Autoneg);
-	phylink_set(phy_support, Pause);
-	phylink_set(phy_support, Asym_Pause);
-
-	linkmode_zero(sfp_support);
-	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
-	/* Some modules support 10G modes as well as others we support.
-	 * Mask out non-supported modes so the correct interface is picked.
-	 */
-	linkmode_and(sfp_support, phy_support, sfp_support);
-
-	if (linkmode_empty(sfp_support)) {
-		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n");
-		return -EINVAL;
-	}
+	if (!port->is_mii)
+		return 0;
 
-	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
+	linkmode_zero(port->supported);
+	phylink_set(port->supported, 1000baseX_Full);
+	phylink_set(port->supported, 1000baseT_Full);
+	phylink_set(port->supported, Autoneg);
+	phylink_set(port->supported, Pause);
+	phylink_set(port->supported, Asym_Pause);
 
-	/* Only 1000Base-X is supported by AR8031/8033 as the downstream SerDes
-	 * interface for use with SFP modules.
-	 * However, some copper modules detected as having a preferred SGMII
-	 * interface do default to and function in 1000Base-X mode, so just
-	 * print a warning and allow such modules, as they may have some chance
-	 * of working.
+	/* This device doesn't really support SGMII. However, do our best
+	 * to be compatible with copper modules (that usually require SGMII),
+	 * in a degraded mode as we only allow 1000BaseT Full
 	 */
-	if (iface == PHY_INTERFACE_MODE_SGMII)
-		dev_warn(&phydev->mdio.dev, "module may not function if 1000Base-X not supported\n");
-	else if (iface != PHY_INTERFACE_MODE_1000BASEX)
-		return -EINVAL;
+	__set_bit(PHY_INTERFACE_MODE_SGMII, port->interfaces);
+	__set_bit(PHY_INTERFACE_MODE_1000BASEX, port->interfaces);
 
 	return 0;
 }
 
-static const struct sfp_upstream_ops at8031_sfp_ops = {
-	.attach = phy_sfp_attach,
-	.detach = phy_sfp_detach,
-	.module_insert = at8031_sfp_insert,
-	.connect_phy = phy_sfp_connect_phy,
-	.disconnect_phy = phy_sfp_disconnect_phy,
-};
-
 static int at8031_parse_dt(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->mdio.dev.of_node;
@@ -841,8 +811,7 @@ static int at8031_parse_dt(struct phy_device *phydev)
 		return ret;
 	}
 
-	/* Only AR8031/8033 support 1000Base-X for SFP modules */
-	return phy_sfp_probe(phydev, &at8031_sfp_ops);
+	return 0;
 }
 
 static int at8031_probe(struct phy_device *phydev)
@@ -1173,6 +1142,7 @@ static struct phy_driver at803x_driver[] = {
 	.set_tunable		= at803x_set_tunable,
 	.cable_test_start	= at8031_cable_test_start,
 	.cable_test_get_status	= at8031_cable_test_get_status,
+	.attach_port		= at8031_attach_port,
 }, {
 	/* Qualcomm Atheros AR8032 */
 	PHY_ID_MATCH_EXACT(ATH8032_PHY_ID),
-- 
2.49.0

On Tue, Jul 22, 2025 at 02:16:16PM +0200, Maxime Chevallier wrote:
> Convert the at803x driver to use the generic phylib SFP handling, via a
> dedicated .attach_port() callback, populating the supported interfaces.
> 
> As these devices are limited to 1000BaseX, a workaround is used to also
> support, in a very limited way, copper modules. This is done by
> supporting SGMII but limiting it to 1G full duplex (in which case it's
> somwhat compatible with 1000BaseX).

Missing e

> +static int at8031_attach_port(struct phy_device *phydev, struct phy_port *port)
>  {

...

> +	if (!port->is_mii)
> +		return 0;

That seems common to all these drivers? Can it be pulled into the
core?

> -	if (iface == PHY_INTERFACE_MODE_SGMII)
> -		dev_warn(&phydev->mdio.dev, "module may not function if 1000Base-X not supported\n");

I think we need to keep this warning. I don't remember the details,
but i think this is the kernel saying the hardware is broken, this
might not work, we will give it a go, but don't blame me if it does
not work. We need to keep this disclaimer.

	Andrew

On Sat, 26 Jul 2025 23:24:36 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Tue, Jul 22, 2025 at 02:16:16PM +0200, Maxime Chevallier wrote:
> > Convert the at803x driver to use the generic phylib SFP handling, via a
> > dedicated .attach_port() callback, populating the supported interfaces.
> > 
> > As these devices are limited to 1000BaseX, a workaround is used to also
> > support, in a very limited way, copper modules. This is done by
> > supporting SGMII but limiting it to 1G full duplex (in which case it's
> > somwhat compatible with 1000BaseX).  
> 
> Missing e
> 
> > +static int at8031_attach_port(struct phy_device *phydev, struct phy_port *port)
> >  {  
> 
> ...
> 
> > +	if (!port->is_mii)
> > +		return 0;  
> 
> That seems common to all these drivers? Can it be pulled into the
> core?

If we pull that into the core, we'll need to add specialised
.attach_port() callbacks in phy_driver, such as

	.attach_mii_port() or .attach_serdes_port()
	.attach_mdi_port()

I'm perfectly OK with that though :)

> 
> > -	if (iface == PHY_INTERFACE_MODE_SGMII)
> > -		dev_warn(&phydev->mdio.dev, "module may not function if 1000Base-X not supported\n");  
> 
> I think we need to keep this warning. I don't remember the details,
> but i think this is the kernel saying the hardware is broken, this
> might not work, we will give it a go, but don't blame me if it does
> not work. We need to keep this disclaimer.

Sure thing, looking at it now I'm not sure why I removed that...

I'll add it back,

Maxime

Hi Russell, Andrew,

On Sat, 26 Jul 2025 23:24:36 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > -	if (iface == PHY_INTERFACE_MODE_SGMII)
> > -		dev_warn(&phydev->mdio.dev, "module may not function if 1000Base-X not supported\n");  
> 
> I think we need to keep this warning. I don't remember the details,
> but i think this is the kernel saying the hardware is broken, this
> might not work, we will give it a go, but don't blame me if it does
> not work. We need to keep this disclaimer.

As I'm preparing for the next iteration, I was wondering if this could
be something we could move into the core.

The series generalizes most of the SFP handling for PHYs, and I
actually don't have a nice spot in at803x to put the warning anymore :)

However what's being said by this warning has nothing specific to
at803x, it applies to any PHY driver (or even, any SFP upstream) that
supports 1000BaseX but does not support SGMII.

The idea is that some modules with a built-in PHY will work when using
1000BaseX as the MII (with of course the limitation that 10/100M won't
ever work), so instead of bailing out when we have an SGMII module on a
1000BaseX SFP cage, we give it a try with a very loud warning that this
is "best effort, probably won't work, don't blame the kernel".

This has been discussed a bit originally here [1]

Is it OK for you if we move that warning into core code ?

Maxime

[1] : https://lore.kernel.org/netdev/20210701231253.GM22278@shell.armlinux.org.uk/