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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!