Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

[Russell King (Oracle)]

On Wed, Jan 08, 2025 at 01:13:35PM +0100, Oleksij Rempel wrote:
> @@ -2508,6 +2369,207 @@ static void lan78xx_remove_irq_domain(struct lan78xx_net *dev)
>  	dev->domain_data.irqdomain = NULL;
>  }
>  
> +static void lan78xx_mac_config(struct phylink_config *config, unsigned int mode,
> +			       const struct phylink_link_state *state)
> +{
> +	struct net_device *net = to_net_dev(config->dev);
> +	struct lan78xx_net *dev = netdev_priv(net);
> +	u32 rgmii_id = 0;
> +	u32 mac_cr = 0;
> +	int ret;
> +
> +	/* Check if the mode is supported */
> +	if (mode != MLO_AN_FIXED && mode != MLO_AN_PHY) {
> +		netdev_err(net, "Unsupported negotiation mode: %u\n", mode);
> +		return;
> +	}
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_INTERNAL:
> +	case PHY_INTERFACE_MODE_GMII:
> +			mac_cr |= MAC_CR_GMII_EN_;
> +		break;

The indentation has gone a bit weird here.

> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		break;

Normally, a MAC should support all RGMII interface modes, because these
define the RGMII delays at the PHY and have little dependence on the
MAC.

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

The mac_config implementation conflicts with this.

> +		link_interface = PHY_INTERFACE_MODE_NA;

This is supposed to be for DSA, not for general use. Is there any reason
you can't pass the real mode that is being used here?

> @@ -2576,7 +2673,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  			return -EIO;
>  		}
>  		phydev->is_internal = true;
> -		dev->interface = PHY_INTERFACE_MODE_GMII;
> +		phydev->interface = PHY_INTERFACE_MODE_INTERNAL;

This needs to be explained.

As for continuing to use fixed-phy, please instead use
phylink_set_fixed_link() instead.

Thanks.

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