Re: [PATCH net-next v5 2/6] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

Russell King (Oracle)
2025-03-24T16:18:17Z

On Wed, Mar 19, 2025 at 09:49:48AM +0100, Oleksij Rempel wrote:
> Convert the LAN78xx driver to use the PHYlink framework for managing
> PHY and MAC interactions.
>
> Key changes include:
> - Replace direct PHY operations with phylink equivalents (e.g.,
> phylink_start, phylink_stop).
> - Introduce lan78xx_phylink_setup for phylink initialization and
> configuration.
> - Add phylink MAC operations (lan78xx_mac_config,
> lan78xx_mac_link_down, lan78xx_mac_link_up) for managing link
> settings and flow control.
> - Remove redundant and now phylink-managed functions like
> `lan78xx_link_status_change`.
> - update lan78xx_get/set_pause to use phylink helpers

I don't think this goes into enough detail - there's some subtle changes
going on in this patch.

> static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
> {
> - struct fixed_phy_status fphy_status = {
> - .link = 1,
> - .speed = SPEED_1000,
> - .duplex = DUPLEX_FULL,
> - };
> struct phy_device *phydev;
> int ret;
>
> phydev = phy_find_first(dev->mdiobus);
> if (!phydev) {
> - netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
> - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
> - if (IS_ERR(phydev)) {
> - netdev_err(dev->net, "No PHY/fixed_PHY found\n");
> - return ERR_PTR(-ENODEV);
> - }
> - netdev_dbg(dev->net, "Registered FIXED PHY\n");
> - dev->interface = PHY_INTERFACE_MODE_RGMII;
> + netdev_dbg(dev->net, "PHY Not Found!! Forcing RGMII configuration\n");

dev->interface is removed.

> ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
> MAC_RGMII_ID_TXC_DELAY_EN_);
> if (ret < 0)
> @@ -2547,7 +2545,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
> netdev_err(dev->net, "no PHY driver found\n");
> return ERR_PTR(-EINVAL);
> }
> - dev->interface = PHY_INTERFACE_MODE_RGMII_ID;

Here too.

> + phydev->interface = PHY_INTERFACE_MODE_RGMII_ID;

I'm not sure why this is being set here - the PHY has been found, but
hasn't had phy_connect*() or phy_attach*() called on it yet (which will
write this member of phy_device.)

> +static int lan78xx_phylink_setup(struct lan78xx_net *dev)
> +{
> + struct phylink_config *pc = &dev->phylink_config;
> + phy_interface_t link_interface;
> + struct phylink *phylink;
> +
> + pc->dev = &dev->net->dev;
> + pc->type = PHYLINK_NETDEV;
> + pc->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 |
> + MAC_100 | MAC_1000FD;
> + pc->mac_managed_pm = true;
> +
> + if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> + phy_interface_set_rgmii(pc->supported_interfaces);
> + link_interface = PHY_INTERFACE_MODE_RGMII_ID;
> + } else {
> + __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> + pc->supported_interfaces);
> + link_interface = PHY_INTERFACE_MODE_INTERNAL;
> + }

Hmm. This seems to me to be a functional change. lan78xx_phy_init() had
a switch() statement that:

1. for ID_REV_CHIP_ID_7801_, calls lan7801_phy_init().

For a fixed PHY, sets dev->interface to PHY_INTERFACE_MODE_RGMII for
a fixed-PHY (and it seems to configure the RGMII interface delays).

For a normal PHY, sets dev->interface to PHY_INTERFACE_MODE_RGMII_ID
and apparently disables the MAC-side RGMII delays.

2. for ID_REV_CHIP_ID_7800_ and ID_REV_CHIP_ID_7850_, uses GMII mode
with an internal PHY. Maybe the internal connection is GMII. Note
that with PHY_INTERFACE_MODE_INTERNAL, phylink will not restrict
the speeds, whereas with PHY_INTERFACE_MODE_GMII it will.

So, I think it would make sense to first make this functional change as
a separate patch.

> if (IS_ERR(phydev)) {
> - netdev_err(dev->net, "lan7801: failed to init PHY: %pe\n",
> - phydev);
> - return PTR_ERR(phydev);
> + struct phylink_link_state state = {
> + .speed = SPEED_1000,
> + .duplex = DUPLEX_FULL,
> + .interface = PHY_INTERFACE_MODE_RGMII,

This member has no effect here. phylink_set_fixed_link() just
reconfigures for a fixed link as if it had been specified by firmware.
It doesn't support changing the interface at this point.

> @@ -2586,7 +2627,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> return -ENODEV;
> }
> phydev->is_internal = true;
> - dev->interface = PHY_INTERFACE_MODE_GMII;
> + phydev->interface = PHY_INTERFACE_MODE_GMII;

Same as the case above with PHY_INTERFACE_MODE_RGMII_ID.

Thanks.

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